Repository navigation
Conversation
Some C2J customizations cannot be expressed as a smithy-build transform that Smithy validates: the s3 bucket strip, the s3control host prefix, page-size member types, shapes renamed in Smithy, and a few model edits Smithy rejects. SmithyServiceCustomization is a per-service hook, selected by sdkId, with a preprocess step on the validated Smithy model and a postprocess step on the intermediate model. SmithyIntermediateModelBuilder runs both. Classes: S3, S3Control, PageSize (cloudfront, route53, glacier), ShapeName, ApiGateway and CloudWatch. No module has a smithy-build.json yet, so no generated client changes.
…s them S3Customization.preprocess threw an NPE for a config without shapeModifiers, such as the customization-free baseline of CustomizationParityTest.
…mithy model The trimmed intermediate model recomputed its endpoint parameters from the rule set, dropping the ones S3Customization adds. The s3 client then lost the DeleteObjectKeys endpoint and auth scheme parameter, and S3EndpointResolverUtils called a builder method that did not exist.
The Smithy model lists NotFound for both operations, where C2J lists NoSuchBucket and NoSuchKey. The Smithy path added a NotFoundException class and named it in throws clauses, though ExceptionTranslationInterceptor turns every HEAD 404 into NoSuchBucketException or NoSuchKeyException.
The injected SourceBucket, SourceKey and SourceVersionId members are unbound, so the translator gave both requests an XML root name and namespace. The marshaller then wrote an empty root element as the request body. C2J takes the root from the model, which has none for these requests.
Leave S3Control, PageSize, ShapeName, ApiGateway and CloudWatch to a later
change, so this one adds the SmithyServiceCustomization hook and its s3
class only.
S3Customization:
- preprocess is three named steps: inject the members customization.config
defines, keep the part-number markers integers, and swap NotFound for the
HeadBucket and HeadObject errors. An injected member with a location other
than header now fails instead of becoming unbound.
- appliesConfig is a fixed key set built from the injected members, plus the
renamed-member required pattern.
- The bucket label is removed with the same replaceOnce as the C2J path,
after renamed members are back on {Bucket}.
- The XML-root step reads only the shapes that have inject entries.
| removeXmlRootOfInjectedBodies(model); | ||
| } | ||
|
|
||
| private static void addEndpointParameters(IntermediateModel model) { |
There was a problem hiding this comment.
This and the addOperationContextParams below could also be put in the general codegen module. S3 is the only service that sets them. On the Smithy path, a service declares its endpoint parameters in the rule set and its context params with the @operationContextParams trait. The builder already reads both.
There was a problem hiding this comment.
I'd say lets keep it S3 specific - There generally shouldn't be cases outside of S3 that would use this (ie, we've said we won't support it for new cases).
There was a problem hiding this comment.
Sorry going back on this a little - do we know there is NO internal usage of these? Effectively removing support for the customization.config values for non-S3 services is potentially breaking.
There was a problem hiding this comment.
yes I checked and there's no internal usage of it.
…izations Resolve the SmithyIntermediateModelBuilder Javadoc conflict with the waiter translation (#7512).
Some of these edits could be smithy-build transforms, so don't say a transform cannot make them.
| */ | ||
| @SdkInternalApi | ||
| public final class SmithyServiceCustomizations { | ||
| private static final List<SmithyServiceCustomization> ALL = Collections.singletonList(new S3Customization()); |
There was a problem hiding this comment.
Should we load these through SPI, which would allow BYO service customizations? (eg for internal customers) - though does make it an extension point/public api....
Edit - I think we leave as is for now, with this as is we can easily extend to SPI at some point in the future if its needed.
| * S3 edits to the Smithy model and the intermediate model: | ||
| * <ul> | ||
| * <li>Injects {@code ExpiresString}, a second member on the {@code Expires} header, and {@code SdkPartType}, an | ||
| * unbound member, as {@code customization.config} defines them.</li> | ||
| * <li>Keeps {@code PartNumberMarker} and {@code NextPartNumberMarker} integers; Smithy requires pagination tokens | ||
| * to be strings.</li> | ||
| * <li>Has HeadBucket and HeadObject throw {@code NoSuchBucket} and {@code NoSuchKey} instead of | ||
| * {@code NotFound}.</li> | ||
| * <li>Signs with the S3 variant of SigV4.</li> | ||
| * <li>Adds the endpoint parameters and operation context parameters from {@code customization.config}.</li> | ||
| * <li>Keeps members that {@code customization.config} renames, such as {@code CopyObjectRequest.Bucket}, optional | ||
| * and on their old URI label.</li> | ||
| * <li>Removes the bucket label and {@code x-id} from request URIs; the endpoint rules place the bucket.</li> | ||
| * <li>Sends no XML body for a request whose only body members are injected, such as | ||
| * {@code CopyObjectRequest}.</li> | ||
| * </ul> |
There was a problem hiding this comment.
This is a lot of individual customizations - I'd argue each customization should be its own class. Yes, its a bit inefficient on smithy model copies, but I think ultimately better isolation of individual customization logic. What do you think?
There was a problem hiding this comment.
Yeh Kotlin puts each customization in its own class too. I was a bit hesitant on this at first mainly because of the extra model copies, since a couple of these edits are only a line or two. But on second thought, separate classes would be better, each edit gets its own tests and can be removed on its own.
| removeXmlRootOfInjectedBodies(model); | ||
| } | ||
|
|
||
| private static void addEndpointParameters(IntermediateModel model) { |
There was a problem hiding this comment.
Sorry going back on this a little - do we know there is NO internal usage of these? Effectively removing support for the customization.config values for non-S3 services is potentially breaking.
| } | ||
|
|
||
| private static void addOperationContextParams(IntermediateModel model) { | ||
| List<CustomOperationContextParam> params = model.getCustomizationConfig().getCustomOperationContextParams(); |
There was a problem hiding this comment.
These are interesting cases where we are still reading and using the customization config... but limiting it to a single service. I'm a bit on the fence on what we want to do for these. For the other S3 customizations we've moved them entirely into this code (eg, the auth hardcodes S3) - I'd argue that the SmithyServiceCustomizations should entirely replace service specific customization.config options instead of reading from from the model.getCustomizationConfig. What do you think?
There was a problem hiding this comment.
Was going to put that as a phase 2 work first, but then did some research and found that the effort is much smaller than I imagined. Made the change to move the needed infomation!
Each s3 edit is its own SmithyServiceCustomization in the service.s3 package, named after what it does and listed by S3Customizations. Restoring the renamed copy-request members stays in S3RemoveBucketFromUri, because the bucket strip needs the restored {Bucket} label.
The classes no longer read customization.config. The injected members, the DeleteObjectKeys endpoint parameter and its DeleteObjects context param are constants. The copy-request steps name CopyObject and UploadPartCopy, whose renames and source members the generic transforms still apply. appliesConfig claims the four renamed members' required keys instead of a pattern.
Adding the DeleteObjects context params now reports a missing operation and existing params separately.
…ation The package holds customizations, and codegen.model.service already names the C2J service-model types. No other change.
Motivation and Context
Some customizations can't be written as smithy-build transforms easily. This PR adds a per-service hook for service specifc customizations. preprocess edits the Smithy model after smithy-build has validated it, and postprocess edits the intermediate model. This pattern follows the same way Kotlin and Go SDKs do for customizations. The PR also implements the hook for s3.
Modifications
SmithyServiceCustomizationinterface incodegen.smithy.customization, an internal per-service hook with two steps.preprocessedits the Smithy model after smithy-build has validated it.postprocessedits the intermediate model. Each customization can also report whichcustomization.configsettings it applies itself, so thesmithy-build.jsongenerator in the next PR can leave those settings out.SmithyServiceCustomizations, the registry of customizations. It holds only s3 for now.SmithyIntermediateModelBuilderruns the matching customizations and keeps the endpoint parameters they add on the trimmed model.codegen.smithy.customization.s3, one class per edit, listed byS3Customizations.S3InjectMembersExpiresString(on theExpiresheader) andSdkPartType(unbound)S3RetypePartNumberMarkersPartNumberMarkerandNextPartNumberMarkeras integersS3ReplaceNotFoundNoSuchBucketandNoSuchKeyinstead ofNotFoundS3SetAuthTypeS3AddDeleteObjectKeysDeleteObjectKeysendpoint parameter and itsDeleteObjectscontext parameterS3RemoveBucketFromUriBucketandKeymembers of CopyObject and UploadPartCopy back on their old URI labels and makes them optional, then removes/{Bucket}andx-idfrom request URIsS3OmitCopyRequestBodyTesting
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.AI tools
License