feat: add support for multi select hash to JMESPath visitor #931
Conversation
0marperez
added
the
no-changelog
| private fun ensureNullGuard(shape: Shape?, expr: String, elvisExpr: String? = null): String = | ||
| if (shape?.isNullable == true) { | ||
| buildString { | ||
| append("?.$expr") | ||
| elvisExpr?.let { append(" ?: $it") } | ||
| } | ||
| } else { | ||
| ".$expr" | ||
| private fun addNullGuard(shape: Shape?, expr: String, elvisExpr: String? = null): String = | ||
| buildString { | ||
| append("?.$expr") | ||
| elvisExpr?.let { append(" ?: $it") } | ||
| } |
There was a problem hiding this comment.
Question: Why are we adding null guards to potentially-non-null expressions? We made the codegen visitor null-aware in response to a waiters bug and I worry that regressing that behavior could introduce new bugs.
There was a problem hiding this comment.
The complexity didn't seem worth it, now that I know the reason behind it it makes sense. It's also very hard to work with. I'll add it back it's just going to take a little while
| val comparison = when (matcher.comparator!!) { | ||
| PathComparator.STRING_EQUALS -> "${actual.identifier} == ${expected.dq()}" | ||
| PathComparator.BOOLEAN_EQUALS -> "${actual.identifier} == ${expected.toBoolean()}" | ||
| PathComparator.ANY_STRING_EQUALS -> "${actual.identifier}?.any { it == ${expected.dq()} } ?: false" | ||
| PathComparator.ANY_STRING_EQUALS -> "(${actual.identifier} as List<String>?)?.any { it == ${expected.dq()} } ?: false" | ||
|
|
||
| // NOTE: the isNotEmpty check is necessary because the waiter spec says that `allStringEquals` requires | ||
| // at least one value unlike Kotlin's `all` which returns true if the collection is empty | ||
| PathComparator.ALL_STRING_EQUALS -> | ||
| "!${actual.identifier}.isNullOrEmpty() && ${actual.identifier}.all { it == ${expected.dq()} }" | ||
| "!(${actual.identifier} as List<String>).isNullOrEmpty() && ${actual.identifier}.all { it == ${expected.dq()} }" | ||
| } |
There was a problem hiding this comment.
Question: Why is this new cast to List<String> necessary?
There was a problem hiding this comment.
Long explanation but when multi select hash makes a selection that selection is stored in an object. Something like this: class Selection<T>(val x: T, val y: T).
The .isNullOrEmpty() function can only be applied to arrays, lists and such.
So at runtime when it was detecting the selection properties as generic (x or y) I was getting errors (because x or y didn't show up as a list, array, etc), that cast fixed them.
| var unwrapExpr: String? = null | ||
|
|
||
| if (member != null) { | ||
| val memberTarget = ctx.model.expectShape(member.target) | ||
| unwrapExpr = when { | ||
| memberTarget.isEnum -> "value" | ||
| memberTarget.isEnumList -> "map { it.value }" | ||
| memberTarget.isEnumMap -> "mapValues { (_, v) -> v.value }" | ||
| memberTarget.isBlobShape || memberTarget.isTimestampShape -> | ||
| throw CodegenException("acceptor behavior for shape type ${memberTarget.type} is undefined") | ||
| else -> null | ||
| } |
There was a problem hiding this comment.
To avoid using a var here, you could use a null-check on member like this:
val unwrapExpr = member?.let {
val memberTarget = ctx.model.expectShape(member.target)
when {
memberTarget.isEnum -> "value"
memberTarget.isEnumList -> "map { it.value }"
memberTarget.isEnumMap -> "mapValues { (_, v) -> v.value }"
memberTarget.isBlobShape || memberTarget.isTimestampShape ->
throw CodegenException("acceptor behavior for shape type ${memberTarget.type} is undefined")
else -> null
}
}| } | ||
|
|
||
| shapeCursor.addLast(member) | ||
| if (member != null) shapeCursor.addLast(member) |
There was a problem hiding this comment.
Same comment here about using the Kotlin null helper methods: member?.let { shapeCursor.addLast(it) }
| val attributes = StringBuilder() | ||
| var i = 0 | ||
| expression.expressions.forEach { | ||
| if (i == expression.expressions.size - 1) { | ||
| attributes.append("val ${it.key}: T") | ||
| } else { | ||
| attributes.append("val ${it.key}: T, ") | ||
| } | ||
| i++ | ||
| } |
There was a problem hiding this comment.
Kotlin has a joinToString function which handles this nicely. The default is a comma-separated string so you don't need to customize the parameters, just the string transformation.
val attributes = expression.expressions.joinToString {
"val ${it.key}: T"
}There was a problem hiding this comment.
Thanks, I wish I knew about this earlier
| val identifiers = StringBuilder() | ||
| i = 0 | ||
| expression.expressions.forEach { | ||
| val identifier = addTempVar(it.key, acceptSubexpression(it.value).identifier) | ||
| if (i == expression.expressions.size - 1) { | ||
| identifiers.append(identifier) | ||
| } else { | ||
| identifiers.append("$identifier, ") | ||
| } | ||
| i++ | ||
| } |
There was a problem hiding this comment.
Same comment about joinToString
| val properties = mutableListOf<String>() | ||
| expression.expressions.forEach { | ||
| properties.add("val ${it.key}: T") | ||
| } | ||
| writer.write("class Selection<T>(${properties.joinToString()})") |
There was a problem hiding this comment.
Nit: Mutable lists are rarely necessary within a block. This, for instance, can be written as a single joinToString translation:
val properties = expression.expressions.joinToString { "val ${it.key}: T" }
writer.write("class Selection<T>($properties)")| val identifiers = mutableListOf<String>() | ||
| expression.expressions.forEach { | ||
| val identifier = addTempVar(it.key, it.value.accept(this).identifier) | ||
| identifiers.add(identifier) | ||
| } | ||
| writer.write("Selection(${identifiers.joinToString()})") |
There was a problem hiding this comment.
Nit: Also could be simplified to avoid mutability:
val identifiers = expression.expressions.joinToString { addTempVar(it.key, it.value.accept(this).identifier) }
writer.write("Selection($identifiers)")| writer.openBlock("val #L = listOfNotNull(", listName) | ||
| writer.openBlock("run {") | ||
|
|
||
| val identifiers = mutableListOf<String>() | ||
| expression.expressions.forEach { | ||
| val identifier = addTempVar(it.key, it.value.accept(this).identifier) | ||
| identifiers.add(identifier) | ||
| } | ||
| writer.write("Selection(${identifiers.joinToString()})") | ||
|
|
||
| writer.closeBlock("},") | ||
| writer.closeBlock(")") |
There was a problem hiding this comment.
Style: Favor withBlock over explicit openBlock/closeBlock:
writer.withBlock("val #L = listOfNotNull(", ")", listName) {
withBlock("run {", "},") {
// ...generate `Selection` class...
}
}
|
SonarCloud Quality Gate failed.
|
Issue #
closes #930
Description of changes
ensureNullGuardis nowaddNullGuardand is added to all temporary val'ssubFieldfunction now allows accessing members of shapes that "are not there" (multi select hash creates anonymous objects with arbitrary attributes that don't show up in the model)anyStringEqualsandallStringEqualsnow casts to a list of strings before comparing (anonymous objects have attributes of generic type)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.