Skip to content

fix: account ID based endpoints #1245

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

Merged
merged 3 commits into from
Feb 25, 2025
Merged

fix: account ID based endpoints #1245

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4fa6596
fix: account ID based endpoints
0marperez Feb 20, 2025
a0aa496
misc: pr feedback v1
0marperez Feb 20, 2025
d3b06f8
misc: pr feedback v2
0marperez Feb 20, 2025
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
25 changes: 16 additions & 9 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 @@ -19,13 +19,11 @@ 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.doubleQuote
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.shapes.ListShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.*

private val suffixSequence = sequenceOf("") + generateSequence(2) { it + 1 }.map(Int::toString) // "", "2", "3", etc.

Expand Down Expand Up @@ -526,11 +524,20 @@ class KotlinJmespathExpressionVisitor(
".$expr"
}

private fun VisitedExpression.getKeys(): String {
val keys = this.shape?.targetOrSelf(ctx.model)?.allMembers
?.keys?.joinToString(", ", "listOf(", ")") { "\"$it\"" }
return keys ?: "listOf<String>()"
}
// TODO: Support maps as objects throughout the visitor
// This visitor assumes JMESPath 'objects' will be classes. DDB assumes JMESPath 'objects' will be maps
Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

Should this be a FIXME instead of a TODO? Do we have a backlog item we can link here (SDK-KT-x)?

Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

Remove note about DDB

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

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

Not sure if it should be a FIXME because the JMESPath visitor has been assuming classes > maps so far and we have had no apparent problems. Other services use JMESPath but this is the first instance I've seen of maps being used as objects. It might be a DDB only quirk or an indication that we took the wrong approach to objects

Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

Either way it sounds like we need a backlog item for this, to at least investigate the discrepancy

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

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

Once that investigation is done we can go back and modify/create a comment here then?

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

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

I think we should keep

// This visitor assumes JMESPath 'objects' will be classes. DDB assumes JMESPath 'objects' will be maps

But change it to mention the discrepancy between the smithy spec and the JMESPath spec for this function

Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

I just think every TODO should have an associated backlog task, otherwise it never gets done

Copy link
Contributor

@ianbotsf ianbotsf Feb 20, 2025

Choose a reason for hiding this comment

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

Question: What does this comment mean? It seems like this visitor now assumes objects can be classes or maps.

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

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

The keys function assumes objects can be classes or maps. The rest of the visitor does not. Hence the comment

Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

Our internal spec says:

Keys function - return a list of the keys in a map. This is the only supported function but is required for binding to key values.

Do we need to support classes?

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

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

The JMESPath spec says

Returns an array containing the keys of the provided object

I guess we can get rid of support for classes if everyone else agrees

Copy link
Contributor

@ianbotsf ianbotsf Feb 20, 2025

Choose a reason for hiding this comment

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

No I believe we should support classes and it's an oversight that we didn't support it before. I was just wondering what else is required. Since nothing seems to be required for this getKeys function anymore I think we should move the TODO up to the class level or wherever's more appropriate for the scope.

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

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

Updated the class with newer comments, let me know what you think

Copy link
Contributor

@ianbotsf ianbotsf Feb 20, 2025

Choose a reason for hiding this comment

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

Question: What's the remaining work to support maps as objects throughout the visitor?

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We would need to modify a decent chunk of the visitor. Anything that deals with objects

private fun VisitedExpression.getKeys(): String =
if (this.shape?.targetOrSelf(ctx.model)?.type == ShapeType.MAP) {
Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

style: unnecessary use of this

"${this.identifier}?.keys?.toList()"
} else {
Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

What happens if the map has keys that aren't strings?

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

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

Good catch, thanks

this
.shape
?.targetOrSelf(ctx.model)
?.allMembers
?.keys
?.joinToString(", ", "listOf(", ")") { it.doubleQuote() }
?: "listOf<String>()"
}

private fun VisitedExpression.getValues(): String {
val values = this.shape?.targetOrSelf(ctx.model)?.allMembers?.keys
Expand Down
29 changes: 28 additions & 1 deletion ...n/software/amazon/smithy/kotlin/codegen/rendering/endpoints/OperationContextParamsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ class OperationContextParamsTest {
}

@Test
fun testKeysFunctionPath() {
fun testKeysFunctionPathWithStructure() {
val input = """
structure TestOperationRequest {
Object: Object
Expand All @@ -114,4 +114,31 @@ class OperationContextParamsTest {

codegen(pathResultType, path, input).shouldContainOnlyOnceWithDiff(expected)
}

@Test
fun testKeysFunctionPathWithMap() {
val input = """
structure TestOperationRequest {
Object: StringMap
}

map StringMap {
key: String
value: String
}
""".trimIndent()

val path = "keys(Object)"
val pathResultType = "stringArray"

val expected = """
@Suppress("UNCHECKED_CAST")
val input = request.context[HttpOperationContext.OperationInput] as TestOperationRequest
val object = input.object
val keys = object?.keys?.toList()
builder.foo = keys
""".formatForTest(" ")

codegen(pathResultType, path, input).shouldContainOnlyOnceWithDiff(expected)
}
}
6 changes: 3 additions & 3 deletions tests/codegen/waiter-tests/model/function-keys.smithy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ use smithy.waiters#waitable
}
]
},
KeysFunctionPrimitivesIntegerEquals: {
Copy link
Contributor

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

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

Why was this integer primitives test removed?

KeysFunctionMapStringEquals: {
acceptors: [
{
state: "success",
matcher: {
output: {
path: "keys(primitives)",
expected: "integer",
path: "keys(maps.strings)",
expected: "key",
comparator: "anyStringEquals"
}
}
Expand Down
9 changes: 5 additions & 4 deletions tests/codegen/waiter-tests/src/test/kotlin/com/test/FunctionKeysTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@

package com.test

import com.test.model.EntityMaps
import com.test.model.EntityPrimitives
import com.test.model.GetFunctionKeysEqualsRequest
import com.test.model.GetFunctionKeysEqualsResponse
import com.test.utils.successTest
import com.test.waiters.waitUntilKeysFunctionPrimitivesIntegerEquals
import com.test.waiters.waitUntilKeysFunctionMapStringEquals
import com.test.waiters.waitUntilKeysFunctionPrimitivesStringEquals
import kotlin.test.Test

Expand All @@ -22,9 +23,9 @@ class FunctionKeysTest {
)

@Test
fun testKeysFunctionPrimitivesIntegerEquals() = successTest(
fun testKeysFunctionMapStringEquals() = successTest(
GetFunctionKeysEqualsRequest { name = "test" },
WaitersTestClient::waitUntilKeysFunctionPrimitivesIntegerEquals,
GetFunctionKeysEqualsResponse { primitives = EntityPrimitives { } },
WaitersTestClient::waitUntilKeysFunctionMapStringEquals,
GetFunctionKeysEqualsResponse { maps = EntityMaps { strings = mapOf("key" to "value") } },
)
Copy link
Contributor

@ianbotsf ianbotsf Feb 20, 2025

Choose a reason for hiding this comment

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

Question: Why did we modify this test case instead of creating a new one? Is the previous behavior no longer valid?

}
Loading