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.

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

Update Jmespath shape traversal codegen to support multi-select lists following projection expressions #3987

Merged

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Jan 25, 2025

Motivation and Context

Adds support for JMESPath multi-select lists following projection expressions such as lists.structs[].[optionalInt, requiredInt] (list projection followed by multi-select lists), lists.structs[<some filter condition>].[optionalInt, requiredInt] (filter projection followed by multi-select lists), and maps.*.[optionalInt, requiredInt] (object projection followed by multi-select lists).

Description

This PR adds support for the said functionality. Prior to the PR, the expressions above ended up the codegen either failing to generate code (for list projection) or generating the incorrect Rust code (for filter & object projection).

All the code changes except for RustJmespathShapeTraversalGenerator.kt are primarily adjusting the existing code based on the updates made to RustJmespathShapeTraversalGenerator.kt.

The gist of the code changes in RustJmespathShapeTraversalGenerator.kt is as follows:

  • generateProjection now supports MultiSelectListExpression on the right-hand side (RHS).
  • Previously, given MultiSelectListExpression on RHS, the map function applied to the result of the left-hand side of a projection expression (regardless of whether it's list, filter, or object projection) returned a type Option<Vec<&T>>, and the map function body used the ? operator to return early as soon as it encountered a field value that was None. That did not yield the desired behavior. Given the snippet lists.structs[].[optionalInt, requiredInt] in the Motivation and Context above for instance, the map function used to look like this:
fn map(_v: &crate::types::Struct) -> Option<Vec<&i32>> {
    let _fld_1 = _v.optional_int.as_ref()?;
    let _fld_2 = _v.required_int;
    let _msl = vec![_fld_1, _fld_2];
    Some(_msl)

This meant if the optional_int in a Struct was None, we lost the chance to access the required_int field when we should've. Instead, the map function now looks like:

fn map(_v: &crate::types::Struct) -> Option<Vec<Option<&i32>>> {
    let _fld_1 = _v.optional_int.as_ref();
    let _fld_2 = Some(_v.required_int);
    let _msl = vec![_fld_1, _fld_2];
    Some(_msl)

This way, the map function body has a chance to access all the fields of Struct even when any of the optional fields in a Struct is None.

Note that the output type of the whole projection expression stays the same before and after this PR; it's just that the inner map function used by the projection expression has been tweaked.

Testing

  • Confirmed existing tests continued working (CI and release pipeline).
  • Added additional JMESPath codegen tests in RustJmespathShapeTraversalGeneratorTest.kt.
  • Confirmed that the updated service model with a JMESPath expression like Items[*].[A.Name, B.Name, C.Name, D.Name][] generated the expected Rust code and behaved as expected.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@ysaito1001 ysaito1001 marked this pull request as ready for review January 27, 2025 18:07
@ysaito1001 ysaito1001 requested a review from a team as a code owner January 27, 2025 18:07
Copy link
Contributor

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -580,6 +647,14 @@ class RustJmespathShapeTraversalGeneratorTest {
"(length(lists.structs[?!(integer < `0`) && integer >= `0` || `false`]) == `5`) == contains(lists.integers, length(maps.structs.*.strings))",
itCompiles,
)

// Derived from https://github.com/awslabs/aws-sdk-rust/blob/8848f51e58fead8d230a0c15f0434b2812825c38/aws-models/auto-scaling.json#L4202
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Didn't realize any services besides DDB were using complex JMESPath

@ysaito1001 ysaito1001 added this pull request to the merge queue Jan 28, 2025
Merged via the queue into main with commit dae6b26 Jan 28, 2025
42 of 44 checks passed
@ysaito1001 ysaito1001 deleted the ysaito/support-multi-select-lists-following-projection branch January 28, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants