Skip to content

sparse codegen #3740

Open
kai-ion wants to merge 9 commits intomainfrom
nullObj
Open

sparse codegen #3740
kai-ion wants to merge 9 commits intomainfrom
nullObj

Conversation

@kai-ion
Copy link
Contributor

@kai-ion kai-ion commented Mar 5, 2026

Issue #, if available:

Description of changes:
Change in codege so that services may return a null object for a type in a collection of objects.

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

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

else if(shape.isList()) {
String type = computeCppTypeInternal(shape.getListMember().getShape(), typeMapping);
if (shape.isSparse()) {
return String.format("Aws::Vector<Aws::Crt::Optional<%s>>", type);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be cleaner and more correct to update type than creating branching return statements i.e.

else if(shape.isList()) {
  String type = computeCppTypeInternal(shape.getListMember().getShape(), typeMapping);
  type = shape.isSparse() ? String.format("Aws::Crt::Optional<%s>",  type) : type
  return String.format("Aws::Vector<%s>", type); 
}

ideally this should go into computeCppTypeInternal but that would require the signature for the method which is a bit much.

the operation on map can follow roughly the same logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out, reverted the branching return and instead update type. Made the same change to update value in list

} else {
context.put("CppViewHelper", CppViewHelper.class);
}
context.put("CppRequestViewHelper", CppViewHelper.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

im not following this, in the event of non-cbor protocols we will run

context.put("CppViewHelper", CppViewHelper.class);
context.put("CppRequestViewHelper", CppViewHelper.class);

why would we add the same class under two different keys? we should only have to add this class to the context once. You likely want to add CppViewHelper regardless of the protocol now, so either you have to reconcile not including it twice, or reconcile why CppCborViewHelper exists and doesn't have the logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So originally I added the Cpprequerstviewhelper class because we can't use a single CppViewHelper class swap (setting it to CppCborViewHelper for CBOR) because request and result shapes intentionally use different type mappings for CBOR. Request classes use base types (int) to preserve API compatibility with existing CBOR services like CloudWatch, while result classes use CBOR types (int64_t).

I agree we should only add the context once, as a workaround for the above issue I made the change to add computeResultCppType(Shape shape, String protocol) which picks the right type mapping internally:

public static String computeResultCppType(Shape shape, String protocol) {
    return "smithy-rpc-v2-cbor".equals(protocol)
            ? computeCppTypeInternal(shape, CORAL_TYPE_TO_CBOR_CPP_TYPE_MAPPING)
            : computeCppTypeInternal(shape, CORAL_TYPE_TO_CPP_TYPE_MAPPING);
}

Templates will now call $CppViewHelper.computeResultCppType($shape, $serviceModel.metadata.protocol)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants