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 all commits
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
39 changes: 29 additions & 10 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,13 +14,14 @@ 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
import software.amazon.smithy.kotlin.codegen.utils.getOrNull
import software.amazon.smithy.kotlin.codegen.utils.toCamelCase
import software.amazon.smithy.model.knowledge.NullableIndex
import software.amazon.smithy.model.shapes.ListShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
Expand Down Expand Up @@ -49,8 +50,6 @@ class KotlinJmespathExpressionVisitor(
) : ExpressionVisitor<VisitedExpression> {
private val tempVars = mutableSetOf<String>()

private val nullableIndex = NullableIndex(ctx.model)

// tracks the current shape on which the visitor is operating
private val shapeCursor = ArrayDeque(listOf(shape))

Expand Down Expand Up @@ -308,14 +307,35 @@ 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 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")
}
}

return ret
private fun index(expression: IndexExpression, parentName: String): VisitedExpression {
shapeCursor.addLast(currentShape.targetOrSelf(ctx.model).targetMemberOrSelf)
val index = if (expression.index < 0) "$parentName.size${expression.index}" else expression.index
return VisitedExpression(addTempVar("index", "$parentName?.get($index)"))
}

private val Shape.isEnumList: Boolean
Expand All @@ -336,8 +356,7 @@ class KotlinJmespathExpressionVisitor(

private val Shape.isNullable: Boolean
get() = this is MemberShape &&
ctx.model.expectShape(target).let { !it.hasTrait<OperationInput>() && !it.hasTrait<OperationOutput>() } &&
nullableIndex.isMemberNullable(this, NullableIndex.CheckMode.CLIENT_ZERO_VALUE_V1_NO_INPUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

was this nullableIndex.isMemberNullable check meant to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was causing issues and the code behaves the same without it

ctx.model.expectShape(target).let { !it.hasTrait<OperationInput>() && !it.hasTrait<OperationOutput>() }

private val Shape.targetMemberOrSelf: Shape
get() = when (val target = targetOrSelf(ctx.model)) {
Expand Down
95 changes: 95 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,92 @@ 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"
}
}
}
]
},
BooleanListIndexNegativeTwoEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "lists.booleans[-2]",
expected: "true",
comparator: "booleanEquals"
}
}
}
]
},
TwoDimensionalBooleanListIndexZeroZeroEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "twoDimensionalLists.booleansList[0][0]",
expected: "true",
comparator: "booleanEquals"
}
}
}
]
},
StructListIndexOneStringsIndexZeroEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "lists.structs[1].strings[0]",
expected: "foo",
comparator: "stringEquals"
}
}
}
]
},
StructListIndexOneSubStructsIndexZeroSubStructPrimitivesBooleanEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "lists.structs[1].subStructs[0].subStructPrimitives.boolean",
expected: "true",
comparator: "booleanEquals"
}
}
}
]
},

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

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

structure TwoDimensionalEntityLists {
booleansList: TwoDimensionalBooleanList,
}

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

list TwoDimensionalBooleanList {
member: BooleanList,
}

list StringList {
member: String,
}
Expand Down
42 changes: 41 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,45 @@ 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, false) } },
GetEntityResponse { lists = EntityLists { booleans = listOf(false, true) } },
)

@Test fun testBooleanListIndexNegativeTwoEquals() = successTest(
WaitersTestClient::waitUntilBooleanListIndexNegativeTwoEquals,
GetEntityResponse { lists = EntityLists { booleans = listOf(false, false) } },
GetEntityResponse { lists = EntityLists { booleans = listOf(false, true) } },
GetEntityResponse { lists = EntityLists { booleans = listOf(true, false) } },
)

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

@Test fun testStructListIndexOneStringsIndexZeroEquals() = successTest(
WaitersTestClient::waitUntilStructListIndexOneStringsIndexZeroEquals,
GetEntityResponse { lists = EntityLists { structs = listOf(Struct { strings = listOf("bar") }, Struct { strings = listOf("bar") }) } },
GetEntityResponse { lists = EntityLists { structs = listOf(Struct { strings = listOf("bar") }, Struct { strings = listOf("foo") }) } },
)

@Test fun testStructListIndexOneSubStructsIndexZeroSubStructPrimitivesBooleanEquals() = successTest(
WaitersTestClient::waitUntilStructListIndexOneSubStructsIndexZeroSubStructPrimitivesBooleanEquals,
GetEntityResponse { lists = EntityLists { structs = listOf(Struct { }, Struct { subStructs = listOf(SubStruct { subStructPrimitives = EntityPrimitives { boolean = false } }) }) } },
GetEntityResponse { lists = EntityLists { structs = listOf(Struct { }, Struct { subStructs = listOf(SubStruct { subStructPrimitives = EntityPrimitives { boolean = true } }) }) } },
)

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