JPERF-729: Create a way to configure Jira admin user password during database setup

[pczuj]

This is draft

• I didn't test if SQL is correct
  • I'd like to use code already present in Hooks for Data Center #103 for that, however I'm not yet 100% sure how to do it correctly
• I believe those changes are not compatible with Hooks for Data Center #103 hance I'd like to consult how I can help to reduce the conflicts and possibly contribute to the Hooks for Data Center #103 with the new password overwrite feature in the new form
• I think that this may be a good opportunity to check if changes in Hooks for Data Center #103 are flexible enough for new features like password overwrite in database
• The password overwrite feature could also be implemented with property -Datlassian.recovery.password=<your-password>. I decided to use the database setup wrapper without any good reason.

[dagguh]

Why is $cwdUserTableName a variable, but cwd_directory_attribute a constant?

[mgrzaslewicz]

Makes sense to remove table name parameters if there is no reason to keep it. @pczuj do you remember why you made it as a parameter?

[pczuj]

The reason was to split the responsibility from a class that knows only the structure of table, but doesn't know it's name. It was also to allow overwrites in case the table name changes in the future or is incorrect for some DBs. We are relying on something that is not really an Jira API here.

We could make the fields also configurable for better portability, but then the class will have nothing specific about Jira DB.

[dagguh]

Then why isn't cwd_directory_attribute also parametrized? YAGNI TBH.

[pczuj]

I think it's because the table name parameter was made by me with first version of the code was created. Then @mgrzaslewicz took over the PR and his addition didn't follow the same approach. That's where the 2 parts of the code differ.

I don't think YAGNI applies here. As mentioned in my previous comment there are at least 2 reasons for the delegation of table name definition (responsibility split + uncertain contract). Both of those are essentially preparing for easier modification, e.g. it would be easier to adapt the table name from the lib consumer repository rather than relying on a new release.

I'd promote a quite from Matrin Fowler here: https://www.martinfowler.com/bliki/Yagni.html

Yagni only applies to capabilities built into the software to support a presumptive feature, it does not apply to effort to make the software easier to modify.

[pczuj]

oh... github has this option that makes your comments invisible... cool option. I was wondering why my messages are ignored. Had to click "finish your review"

[dagguh]

Here's a PR with some of my suggestions: pczuj#1

[pczuj]

I believe String#contains on the SQL query output is not the correct contract for choosing the password encryption. It may be good enough simplification, however I'd suggest to be honest about this hack in the code.

Also in the idea of SOLID I'd personally split this class into smaller modules:

• SshSqlQueryOutputProvider(sqlClient: SshSqlClient, sqlQueryProvider: () -> String) : (ssh: SshConnection) -> String
• JiraDirectoryAttributeSqlQueryProvider(schema: String, attribute_name: String): () -> String
• JiraDirectoryAttributeSqlQueryOutputParser() : (String) -> List<DirectoryAttribute>
• JiraDirectoryAttributePicker(directoryId: String) : (List<DirectoryAttribute>) -> DirectoryAttribute
• DirectoryAttribute { val directoryId: String, val name: String, val value: String }
• JiraDirectoryAttributeBasedPasswordPicker(passwordMapping: Map<String, String>, defaultHandler: () -> String): (DirectoryAttribute) -> String

Glueing this together can be done in different ways. The idea is to represent every behavior as class for better modularity. Ideally we could allow every module to be overridable, however the example below doesn't do that.

SshSqlQueryExecutor<T>(
    sqlQueryOutputProvider: (ssh: SshConnection) -> String,
    sqlQueryOutputTransformer: (String) -> T
) : (ssh: SshConnection) -> T

fun <I, T, O> ((I) -> T).map(transformation: (T) -> O) = { input: I -> transformation(this(input)) }

CrowdEncryptedPasswordProvider(
    sqlClient: SshSqlClient,
    jiraDatabaseSchemaName: String,
    passwordPlainText: String,
    passwordEncryptedWithAtlassianSecurityPasswordEncoder: String
) : (ssh: SshConnection) -> String by SshSqlQueryExecutor(
    sqlQueryOutputProvider = SshSqlQueryOutputProvider(
        sqlClient = sqlClient,
        sqlQueryProvider = JiraDirectoryAttributeSqlQueryProvider(
            schema = jiraDatabaseSchemaName,
            attribute_name = "user_encryption_method"
        )
    )
    sqlQueryOutputTransformer = JiraDirectoryAttributeSqlQueryOutputParser()
        .map(JiraDirectoryAttributePicker("1"))
        .map(
            JiraDirectoryAttributeBasedPasswordPicker(
                passwordMapping = listOf(
                    "plaintext" to passwordPlainText
                    "atlassian-security" to passwordEncryptedWithAtlassianSecurityPasswordEncoder
                ),
                defaultHandler = { throw RuntimeException("Unknown jira user password encryption type") }
            )
        )
)

Decide yourself about the details and if such change could be better or not.

[dagguh]

We can generate infinite abstractions. The question is: "which ones are useful?". Each abstraction should have a practical (not purely hypothetical) purpose.

[mgrzaslewicz]

IMHO this is overcomplicating simple job and premature optimization. The example provided is hard to understand, at least for me.
Even extracting JiraUserEncryptedPasswordProvider from JiraUserPasswordOverridingDatabase has some drawback and does not make code easier to understand.
It is already overridable, if someone wants to provide own implementation of password provider one can do it easily.

String result from db contains also some additional lines including header. You can prepare here super precise implementation and do not use String.contains. But what would be the benefit?
There are 2 enums in the library providing crowd encryption. If that changes, any implementation will have to be changed.

[pczuj]

We can generate infinite abstractions.

I'm not pointing abstractions here. There is limited number of responsibilities and this class has more than one. At least query execution and excetional result condition handling IMHO should be split.

If we are assuming a contract that is not given to us explicitly I'd prefer to have this contract isolated as much as possible. In our case we are assuming that:

• The query will return only one entry or the first entry returned will be the one that we are interested in - In the example code I added explicit picker for the entry that we are interested in. We could implement it as "take the first one" and document it as a hack. There are also other ways to implement it more explicitly.
• The query will return header not containing the "plaintext" or "atlassian-security" - In the example code I added parser for SQL output. This one also may be a hack that checks exactly for String#contains and maps it e.g. to an enum class. Such class again would allow us to say in javadoc that it's a hack to reduce cost of the implementation.

IMHO this is overcomplicating simple job and premature optimization.

Ability to split it to many responsibilities proves that this is not so simple and fact that I'm able to use this way to spot some implicit contracts that the code starts to rely on makes it not premature optimization, but rather quality assurance.

Still I'm not insisting in chaging it if both of you don't see value in it. So far my understanding of SRP was that we should split as much as possible and that SRP is something that we are following. Possibly one of those is incorrect.

[pczuj]

Rant / Topic not specific to the PR, but still related to the changes:

I find it somewhat risky to use the industry standard ways of junit test setup. It promotes practices of hiding what is actually happening as part of the code execution. I know that follwoing industry standard reduced WTF/min, however what if the standard itself is incorrect?

An alternative way would be to wrap the test class state into an inner class of the test and return this test state as part of setup() factory method. It would cost a single line in each test, however there would be no doubt what the execution order is and there would be no usage of lateinit var, so there is less chance that new contributors will repeat it in other places of the code now knowing that we are trying to avoid mutability and inexplicit nullability.

Actionable around it is not required, however I'd like to know you thoughts on that.

[dagguh]

It's about deduplicating setup code, bit more complex to read, but less to read.

[mgrzaslewicz]

I've checked 'feelings' around below implementation and the benefit seem to be tiny.
Anyway, I think it might be worth implementing.

class JiraUserPasswordOverridingDatabaseTest {

    private val jira = URI("http://localhost/")
    private val samplePlainTextPassword = "plain text password"
    private val expectedEncryptedPassword = "*******"

    data class TestContext(
        val database: Database,
        val underlyingDatabase: RememberingDatabase,
        val sshConnection: RememberingSshConnection,
        val sqlClient: MockSshSqlClient
    )

    private lateinit var testContext: TestContext

    @Before
    fun setup() {
        val db = RememberingDatabase()
        val sqlClient = MockSshSqlClient()
        testContext = TestContext(
            underlyingDatabase = db,
            sshConnection = RememberingSshConnection(),
            sqlClient = sqlClient,
            database = db
                .overrideAdminPassword(
                    adminPasswordPlainText = samplePlainTextPassword,
                    adminPasswordEncrypted = expectedEncryptedPassword
                )
                .sqlClient(sqlClient)
                .schema("jira")
                .jiraUserEncryptedPasswordProvider(object : JiraUserEncryptedPasswordProvider {
                    override fun getEncryptedPassword(ssh: SshConnection) = expectedEncryptedPassword
                })
                .build()
        )
    }

    @Test
    fun shouldSetupUnderlyingDatabase() {
        // when
        testContext.database.setup(testContext.sshConnection)
        testContext.database.start(jira, testContext.sshConnection)
        // then
        assertThat(testContext.underlyingDatabase.isSetup).`as`("underlying database setup").isTrue()
    }
}

[mgrzaslewicz]

or this way:

    @Test
    fun shouldSetupUnderlyingDatabase() {
        with(testContext) {
            // when
            database.setup(sshConnection)
            database.start(jira, sshConnection)
            // then
            assertThat(underlyingDatabase.isSetup).`as`("underlying database setup").isTrue()
        }
    }

but both feels weird, maybe because I've never seen nor used such way to implement test just to avoid lateinit var

[mgrzaslewicz]

So you mean something like that, which requires no knowledge about any magic done by adding @before annotation

class JiraUserPasswordOverridingDatabaseTest {

    private val jira = URI("http://localhost/")
    private val samplePlainTextPassword = "plain text password"
    private val expectedEncryptedPassword = "*******"

    data class TestContext(
        val database: Database,
        val underlyingDatabase: RememberingDatabase,
        val sshConnection: RememberingSshConnection,
        val sqlClient: MockSshSqlClient
    )

    fun setup(): TestContext {
        val db = RememberingDatabase()
        val sqlClient = MockSshSqlClient()
        return TestContext(
            underlyingDatabase = db,
            sshConnection = RememberingSshConnection(),
            sqlClient = sqlClient,
            database = db
                .overrideAdminPassword(
                    adminPasswordPlainText = samplePlainTextPassword,
                    adminPasswordEncrypted = expectedEncryptedPassword
                )
                .sqlClient(sqlClient)
                .schema("jira")
                .jiraUserEncryptedPasswordProvider(object : JiraUserEncryptedPasswordProvider {
                    override fun getEncryptedPassword(ssh: SshConnection) = expectedEncryptedPassword
                })
                .build()
        )
    }

    @Test
    fun shouldSetupUnderlyingDatabase() {
        val testContext = setup()
        with(testContext) {
            // when
            database.setup(sshConnection)
            database.start(jira, sshConnection)
            // then
            assertThat(underlyingDatabase.isSetup).`as`("underlying database setup").isTrue()
        }
    }
}

[pczuj]

Yes @mgrzaslewicz exactly that. The junit magic seems to be unnecessary for me.

Not sure if we should follow this practice in this PR though. Maybe the risk that someone new will come and do it with @Setup anyway is the reason to not go for it. I think that having 2 ways in the code may be worse then having only the worse version of the solution. Possibly a separate PR that changes every occurance of @Setup to something like this would be correct way.

[pczuj]

Nit: Logging responsibility could be split to a decorator on this class.

[mgrzaslewicz]

What would be the benefit?

[pczuj]

There are multiple benefits:

• Decoupling logging from a class ensures that your code is observable
  • It's possible to create code that will replace logging with something easier to understand by machine, e.g. event system.
  • It's possible to write unit tests on the same signals as would trigger log lines seen by the user, i.e. you don't need to mock logger to use same signals.
• You could easily remove the logger code from the execution path by removing the decorator from the initialisation. It could be done dynamically.
• You could unit test the logger responsibility.
• It spreads the foundation of the good practice of keeping things SRP.

Again I'm not insisting, especially with this one, because our practice was so far to not decouple logger from other business logic. Still I wanted to point it out to see your thoughts on it. I personally would prefer us to follow the decoupling practices, because I see how not following it damages the flexibility of our implementations. I don't believe in popular thinking that everything can be rewritten to match the new requirements - from my experience it never happens. However we can write code that is modular and later one be able to initialise it differently to achieve new requirements.

[mgrzaslewicz]

I understand all the benefits, but do we really need them?
Was there a single time in this library we had to test what's being logged or a need to make it configurable?
I don't feel adding functionality because it might be needed some day is a good thing.

[dagguh]

@pczuj I think these are interesting dev experiments, they deserve to be explored. However, we shouldn't mix them inside a big and overdue feature PR.

[dagguh]

Returning the builder opens up ways to override other params, thanks 👍