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.

Sign up for GitHub

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

Jump to bottom

feat: add support for sub-expressions and list indexing in JMESPath #912

Merged
merged 2 commits into from
Aug 14, 2023
Merged

feat: add support for sub-expressions and list indexing in JMESPath #912

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31a6db0
Added support for sub-expressions and list indexing
0marperez Aug 9, 2023
9d2ae0a
Bug fixes, negative indexing and more robust tests
0marperez Aug 11, 2023
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
30 changes: 25 additions & 5 deletions ...oftware/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import software.amazon.smithy.kotlin.codegen.core.CodegenContext
import software.amazon.smithy.kotlin.codegen.core.KotlinWriter
import software.amazon.smithy.kotlin.codegen.core.RuntimeTypes
import software.amazon.smithy.kotlin.codegen.core.withBlock
import software.amazon.smithy.kotlin.codegen.model.*
import software.amazon.smithy.kotlin.codegen.model.hasTrait
import software.amazon.smithy.kotlin.codegen.model.isEnum
import software.amazon.smithy.kotlin.codegen.model.targetOrSelf
import software.amazon.smithy.kotlin.codegen.model.traits.OperationInput
import software.amazon.smithy.kotlin.codegen.model.traits.OperationOutput
import software.amazon.smithy.kotlin.codegen.utils.dq
Expand Down Expand Up @@ -308,16 +310,34 @@ class KotlinJmespathExpressionVisitor(
}

override fun visitSubexpression(expression: Subexpression): VisitedExpression {
val left = expression.left.accept(this)
val leftName = expression.left.accept(this).identifier

val ret = when (val right = expression.right) {
is FieldExpression -> subfield(right, left.identifier)
return when (val right = expression.right) {
is FieldExpression -> subfield(right, leftName)
is IndexExpression -> index(right, leftName)
is Subexpression -> subexpression(right, leftName)
else -> throw CodegenException("Subexpression type $right is unsupported")
}
}

private fun subexpression(expression: Subexpression, parentName: String): VisitedExpression {
val leftName = when (val left = expression.left) {
is FieldExpression -> subfield(left, parentName).identifier
is Subexpression -> subexpression(left, parentName).identifier
else -> throw CodegenException("Subexpression type $left is unsupported")
Comment on lines +322 to +324
Copy link
Contributor

Choose a reason for hiding this comment

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

Could left also be an IndexExpression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it could but it really shouldn't be. The expression would be trying to access an index in nothing i.e. "[2]..."

}

return ret
return when (val right = expression.right) {
is FieldExpression -> subfield(right, leftName)
is Subexpression -> subexpression(right, leftName)
is IndexExpression -> index(right, leftName)
else -> throw CodegenException("Subexpression type $right is unsupported")
}
}

private fun index(expression: IndexExpression, parentName: String): VisitedExpression =
VisitedExpression(addTempVar("index", "$parentName?.get(${expression.index})"))

private val Shape.isEnumList: Boolean
get() = this is ListShape && ctx.model.expectShape(member.target).isEnum

Expand Down
53 changes: 53 additions & 0 deletions tests/codegen/waiter-tests/model/waiter-operations.smithy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,50 @@ service WaitersTestService {
]
},

// list indexing
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Would be good to also test a more involved index expression like the struct list list.structs[1].strings[0]

BooleanListIndexZeroEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "lists.booleans[0]",
expected: "true",
comparator: "booleanEquals"
}
}
}
]
},
BooleanListIndexOneEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "lists.booleans[1]",
expected: "true",
comparator: "booleanEquals"
}
}
}
]
},
TwoDimensionalBooleanListIndexZeroZeroEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "twoDimensionalLists.booleansList[0][0]",
expected: "true",
comparator: "booleanEquals"
}
}
}
]
},

// anyStringEquals
StringListAnyStringEquals: {
acceptors: [
Expand Down Expand Up @@ -764,6 +808,7 @@ structure GetEntityRequest {
structure GetEntityResponse {
primitives: EntityPrimitives,
lists: EntityLists,
twoDimensionalLists: TwoDimensionalEntityLists,
maps: EntityMaps,
}

Expand Down Expand Up @@ -800,6 +845,10 @@ structure EntityLists {
structs: StructList,
}

structure TwoDimensionalEntityLists {
booleansList: TwoDimensionalBooleanList,
}

structure EntityMaps {
booleans: BooleanMap,
strings: StringMap,
Expand All @@ -823,6 +872,10 @@ list BooleanList {
member: Boolean,
}

list TwoDimensionalBooleanList {
member: BooleanList,
}

list StringList {
member: String,
}
Expand Down
22 changes: 21 additions & 1 deletion tests/codegen/waiter-tests/src/test/kotlin/com/test/WaiterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import com.test.model.Enum
import com.test.waiters.*
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.runTest
import kotlin.test.*
import kotlin.test.Test
import kotlin.test.assertEquals

@OptIn(ExperimentalCoroutinesApi::class)
class WaiterTest {
Expand Down Expand Up @@ -107,6 +108,25 @@ class WaiterTest {
GetEntityResponse { primitives = EntityPrimitives { intEnum = IntEnum.One } },
)

// list indexing
@Test fun testBooleanListIndexZeroEquals() = successTest(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Would be good to have some failure tests (e.g. doesn't match, index out of bounds, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Smithy wouldn't allow out of bounds indexing but I tried. I would get an error message:

comparator must return a boolean type, but this acceptor was statically determined to return a null type.

WaitersTestClient::waitUntilBooleanListIndexZeroEquals,
GetEntityResponse { lists = EntityLists { booleans = listOf(false) } },
GetEntityResponse { lists = EntityLists { booleans = listOf(true) } },
)

@Test fun testBooleanListIndexOneEquals() = successTest(
WaitersTestClient::waitUntilBooleanListIndexOneEquals,
GetEntityResponse { lists = EntityLists { booleans = listOf(false, false) } },
GetEntityResponse { lists = EntityLists { booleans = listOf(true, true) } },
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This doesn't test that the correct index is being observed since both values in the list are changing at the same time. I recommend testing [false, false], [true, false], and then finally [false, true].


@Test fun testTwoDimensionalBooleanListIndexZeroZeroEquals() = successTest(
WaitersTestClient::waitUntilTwoDimensionalBooleanListIndexZeroZeroEquals,
GetEntityResponse { twoDimensionalLists = TwoDimensionalEntityLists { booleansList = listOf(listOf(false)) } },
GetEntityResponse { twoDimensionalLists = TwoDimensionalEntityLists { booleansList = listOf(listOf(true)) } },
)

// anyStringEquals
@Test fun testStringListAnyListStringEquals() = successTest(
WaitersTestClient::waitUntilStringListAnyStringEquals,
Expand Down
Loading