fix: account ID based endpoints #1245

0marperez · 2025-02-20T14:09:26Z

Issue #

N/A

Description of changes

Adds support for Kotlin maps as JMESPath objects to the keys function

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lauzadis · 2025-02-20T15:26:06Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+    // TODO: Support maps as objects throughout the visitor
+    // This visitor assumes JMESPath 'objects' will be classes. DDB assumes JMESPath 'objects' will be maps


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

Remove note about DDB

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

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

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

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

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

lauzadis · 2025-02-20T15:28:21Z

tests/codegen/waiter-tests/model/function-keys.smithy

            }
        ]
    },
-    KeysFunctionPrimitivesIntegerEquals: {


Why was this integer primitives test removed?

lauzadis · 2025-02-20T15:29:44Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+        if (this.shape?.targetOrSelf(ctx.model)?.type == ShapeType.MAP) {
+            "${this.identifier}?.keys?.toList()"
+        } else {


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

Good catch, thanks

lauzadis · 2025-02-20T15:31:02Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+    // TODO: Support maps as objects throughout the visitor
+    // This visitor assumes JMESPath 'objects' will be classes. DDB assumes JMESPath 'objects' will be maps
+    private fun VisitedExpression.getKeys(): String =
+        if (this.shape?.targetOrSelf(ctx.model)?.type == ShapeType.MAP) {


style: unnecessary use of this

ianbotsf · 2025-02-20T15:27:26Z

tests/codegen/waiter-tests/src/test/kotlin/com/test/FunctionKeysTest.kt

    @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") } },
    )


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

ianbotsf · 2025-02-20T15:29:52Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+    // TODO: Support maps as objects throughout the visitor
+    // This visitor assumes JMESPath 'objects' will be classes. DDB assumes JMESPath 'objects' will be maps


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

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

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?

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

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.

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

ianbotsf · 2025-02-20T15:30:57Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+    // TODO: Support maps as objects throughout the visitor
+    // This visitor assumes JMESPath 'objects' will be classes. DDB assumes JMESPath 'objects' will be maps


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

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

lauzadis · 2025-02-20T18:50:06Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+Smithy spec: https://smithy.io/2.0/additional-specs/rules-engine/parameters.html#smithy-rules-operationcontextparams-trait
+JMESPath spec: https://jmespath.org/specification.html#keys
+
+TODO: Proactively support JMESPath objects as Kotlin maps throughout thr entire visitor if more instances of this behavior start popping up


"Proactively" and "if more instances of this behavior start popping up" are inconsistent. To be proactive we would need to identify any gaps and fix them now

github-actions · 2025-02-20T18:58:29Z

Affected Artifacts

No artifacts changed size

fix: account ID based endpoints

4fa6596

0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Feb 20, 2025

This comment has been minimized.

Sign in to view

0marperez marked this pull request as ready for review February 20, 2025 14:37

0marperez requested a review from a team as a code owner February 20, 2025 14:37

lauzadis reviewed Feb 20, 2025

View reviewed changes

ianbotsf reviewed Feb 20, 2025

View reviewed changes

misc: pr feedback v1

a0aa496

This comment has been minimized.

Sign in to view

lauzadis reviewed Feb 20, 2025

View reviewed changes

misc: pr feedback v2

d3b06f8

0marperez requested review from ianbotsf and lauzadis February 24, 2025 15:53

lauzadis approved these changes Feb 24, 2025

View reviewed changes

0marperez merged commit 80d395b into main Feb 25, 2025
16 checks passed

0marperez deleted the fix-acc-id-ep branch February 25, 2025 15:38

		// TODO: Support maps as objects throughout the visitor
		// This visitor assumes JMESPath 'objects' will be classes. DDB assumes JMESPath 'objects' will be maps

fix: account ID based endpoints #1245

fix: account ID based endpoints #1245

Uh oh!

Conversation

0marperez commented Feb 20, 2025

Issue #

Description of changes

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0marperez Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 20, 2025

Affected Artifacts

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0marperez Feb 20, 2025 •

edited

Loading