Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JQL refactoring draft #23

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.atlassian.performance.tools.jiraactions.api.jql

import com.atlassian.performance.tools.jiraactions.api.SeededRandom
import com.atlassian.performance.tools.jiraactions.api.page.IssuePage

interface JqlContext {
Copy link
Contributor

@sebapawlak sebapawlak Jul 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like and public SPI. Historically we had many issues with SPI written in Kotlin and how it interoperated with Java and Java interfaces and their default methods. I would suggest writhing this part in Java which should not be costly but may pay off in case we want to extend the SPI in the future without breaking the compatibility.

fun seededRandom(): SeededRandom
fun issuePage(): IssuePage?
}

interface JqlSupplier {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to reduce the number of public interfaces? It seems that our API grows quickly.

Copy link
Contributor Author

@drspartez drspartez Jul 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supplier is replacement for PrescriptionType which is public and could be removed only witch major change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to have many small interfaces in the API. It's much better than having a few huge ones.

fun get(context: JqlContext): Jql?
fun uniqueName(): String
}

interface Jql {
fun query(): String
fun supplier(): JqlSupplier
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.atlassian.performance.tools.jiraactions.api.memories

import com.atlassian.performance.tools.jiraactions.api.jql.Jql
import com.atlassian.performance.tools.jiraactions.api.page.IssuePage
import java.util.function.Predicate

interface JqlMemory2 : Memory<Jql> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this a replacement for JqlMemory, shouldn't that one be deprecated?
If it's too early to deprecate, then maybe it's too early to put in replacements? New API works best if it was tried in some way, e.g.:

  • unit/integration tests within the module
  • equivalent API used outside of the module


fun observe(issuePage: IssuePage)
fun recall(filter: Predicate<Jql>): Jql?

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package com.atlassian.performance.tools.jiraactions.api.memories.adaptive

import com.atlassian.performance.tools.jiraactions.api.SeededRandom
import com.atlassian.performance.tools.jiraactions.api.jql.Jql
import com.atlassian.performance.tools.jiraactions.api.jql.JqlSupplier
import com.atlassian.performance.tools.jiraactions.api.memories.JqlMemory
import com.atlassian.performance.tools.jiraactions.api.memories.JqlMemory2
import com.atlassian.performance.tools.jiraactions.api.memories.adaptive.jql.JqlPrescription
import com.atlassian.performance.tools.jiraactions.api.memories.adaptive.jql.JqlPrescriptions
import com.atlassian.performance.tools.jiraactions.api.page.IssuePage
import com.atlassian.performance.tools.jiraactions.jql.BuiltInJQL
import com.atlassian.performance.tools.jiraactions.jql.JqlContextImpl
import org.apache.logging.log4j.LogManager
import org.apache.logging.log4j.Logger
import java.util.function.Predicate

class AdaptiveJqlMemory2(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this class is public? We have already public interface JqlMemory2.
Should we create some sort of factory that returns the instance of JqlMemory2 in this case AdaptiveJqlMemory2?
What should be a general idea here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general classes with "2" suffix are replacements, for existing which will be deprecated and removed with major change, so in some time number of interfaces will be the same as before refactoring

Copy link
Contributor Author

@drspartez drspartez Jul 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lipinskipawel for more invasive refactoring You can look here
https://github.com/drspartez/jira-actions/tree/issue/memories-refactoring
A lot of implementation is hidden there. And memory interfaces are reduced to 2 pieces

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was more about the Factory class, I do understand the "2" suffix.
Anyway, good to see Factory in the commit you sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. So concern of this change is to replace "String" jql representation with something else which is more relevant (so all JQL interfaces ore for this reason). Other stuff remains almost untouched (so we still offer Adaptive memories implementations as usual).
In second approach (https://github.com/drspartez/jira-actions/tree/issue/memories-refactoring) i changed the fashion we provide adaptive memories (hidden implementation and factories), also reduced number of actual memory interfaces to 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this class is public?

The old AdaptiveJqlMemory was also API, so it's perfectly fine for the replacement to be public as well.
It was published to make custom scenarios easier to write.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create some sort of factory that returns the instance of JqlMemory2 in this case AdaptiveJqlMemory2?

Factories cost complexity and must have additional value to be justified. AFAIK across all JPT libs we expose only one factory: com.atlassian.performance.tools.report.api.result.RawCohortResult.Factory. We did that to offer different kinds of RawCohortResult without allowing custom subtypes.

Why did we forbid custom subtypes? Because we needed to add methods to the (old) CohortResult, but we didn't have a pattern for that. So we created a replacement (just like JqlMemory2) and we wanted to protect ourselves from this problem in the future. We decided nobody should need to implement this interface. In short, we wanted RawCohortResult to be API, but not SPI.

The question is - do we want to allow custom implementations of JqlMemory2? If so, let's go for a factory, else public constructors/builders.

private val random: SeededRandom
) : JqlMemory2 {

private val logger: Logger = LogManager.getLogger(this::class.java)

companion object {
fun getBuiltInJqlSuppliers(): List<JqlSupplier> = BuiltInJQL.values().toList()
}

private val preparedQueries = mutableListOf<Jql>();

init {
BuiltInJQL.RESOLVED.get(JqlContextImpl(random))?.let { preparedQueries.add(it) }
BuiltInJQL.GENERIC_WIDE.get(JqlContextImpl(random))?.let { preparedQueries.add(it) }
}

private val availableJqlSuppliers = mutableSetOf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory impls are implicitly allowed to be not thread-safe, because each VU has its own memories.
It's a nice opportunity to make that assumption explicit and mark AdaptiveJqlMemory2 as @NotThreadSafe.

BuiltInJQL.PRIORITIES,
BuiltInJQL.PROJECT,
BuiltInJQL.ASSIGNEE,
BuiltInJQL.REPORTERS,
BuiltInJQL.PROJECT_ASSIGNEE,
BuiltInJQL.GIVEN_WORD
)

override fun observe(issuePage: IssuePage) {
val jql = availableJqlSuppliers.asSequence()
.map { it.get(JqlContextImpl(random, issuePage)) }
.filter { it != null }
.firstOrNull()

jql?.let {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-code-style 😉
jql can be inlined and therefore chained
and then the it can be renamed to jql

logger.debug("Rendered a new jql query: <<${it.query()!!}>>")
preparedQueries.add(it)
availableJqlSuppliers.remove(it.supplier())
}
}

override fun recall(): Jql? {
return random.pick(preparedQueries)
}

override fun remember(memories: Collection<Jql>) {
preparedQueries.addAll(memories)
}

override fun recall(filter: Predicate<Jql>): Jql? {
return random.pick(preparedQueries.filter {filter.test(it) }.toList())
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package com.atlassian.performance.tools.jiraactions.jql

import com.atlassian.performance.tools.jiraactions.api.SeededRandom
import com.atlassian.performance.tools.jiraactions.api.jql.Jql
import com.atlassian.performance.tools.jiraactions.api.jql.JqlContext
import com.atlassian.performance.tools.jiraactions.api.jql.JqlSupplier
import com.atlassian.performance.tools.jiraactions.api.page.IssuePage

internal class JqlImpl(private val query: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"JQL implementation". What makes this one different from the other implementations?
If none other exist, how could they differ?

It seems that the interface allows dynamic query and supplier logic, but this impl simply reuses the provided fields. Suggestions: StaticJql, SuppliedJql.

private val supplier: JqlSupplier): Jql {
override fun query(): String = this.query
override fun supplier(): JqlSupplier = this.supplier
}

internal class JqlContextImpl(private val rnd: SeededRandom, private val page: IssuePage?) : JqlContext {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rnd - Research and Development? :trollface:

constructor(rnd: SeededRandom) : this(rnd, null)
override fun seededRandom(): SeededRandom = this.rnd
override fun issuePage(): IssuePage? = this.page
}

internal enum class BuiltInJQL(private val sup: (JqlContext) -> String?) : JqlSupplier {
GENERIC_WIDE({ "text ~ \"a*\" order by summary" }),
RESOLVED({ "resolved is not empty order by description" }),
PRIORITIES({
ctx -> ctx.issuePage()?.getPossiblePriorities()
?.let { ctx.seededRandom().pickMany(it, 3) }
?.joinToString()
?.let { "priority in ($it) order by reporter" }
}),
PROJECT({
ctx -> ctx.issuePage()?.getProject()?.let { "project = ${it.key} order by status" }
}),
ASSIGNEE({
ctx -> ctx.issuePage()?.getAssignee()?.let { "assignee = $it order by project" }
}),
REPORTERS({
ctx -> ctx.issuePage()?.getReporter()?.let { "reporter was $it order by description" }
}),
PROJECT_ASSIGNEE({
ctx -> ctx.issuePage()?.let {
val userName = it.getAssignee()
val project = it.getProject()
if (userName != null && project != null) {
"project = ${project.key} and assignee = $userName order by reporter"
} else {
null
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's something wonky about the indents here


}),

/**
* Creates a JQL based on the words used in fields of this issue.
* What we need is to pick a word that will get us a non-empty jql, preferably that will return more issues in
* the search result.
* Warning! Current implementation will work only with locales that use Latin alphabet.
*/
GIVEN_WORD({
ctx -> ctx.issuePage()?.let {
val whitespacesPattern = Regex("\\s")
val lettersOnly = Regex("\\w+")

// in order to find a word that's neither long or short, with a chance of it being just a regular word,
// we're looking for a word with three vowels.
val description = it.getDescription()
val summary = it.getSummary()

val vowels = setOf('a', 'e', 'i', 'o', 'u', 'y')
fun numVowels(s: String): Int {
return s.toCharArray().asSequence().filter { vowels.contains(it) }.count()
}

"$description $summary"
.split(whitespacesPattern)
.filter { it.isNotBlank() }
.shuffled(ctx.seededRandom().random)
.asSequence()
.filter { it.matches(lettersOnly) }
.filter { numVowels(it) == 3 }
.firstOrNull()
?.let { """text ~ "$it" order by key""" }
}
});

override fun uniqueName(): String = this.name
override fun get(context: JqlContext): Jql? = sup.invoke(context)?.let { JqlImpl(it, this) }
}