feat: implement automatic prefix trimming for route53 resource ids #655
Conversation
|
A new generated diff is ready to view: __generated-main...__generated-feat-route53-trimprefix |
| override val name: String | ||
| get() = "TrimResourcePrefix" | ||
|
|
||
| override val order: Byte = -128 |
There was a problem hiding this comment.
Question: Why does this need to execute first?
There was a problem hiding this comment.
It makes sense in my mind that doing any modification on the user input would occur before serialization (so basically, it should execute first).
There was a problem hiding this comment.
Important to note that the middleware itself also is ordered by phase. The initialization phase precedes serialization so it wouldn't matter what order they are in for codegen.
| write("val prefix = #S.toRegex()", "^/?.*?/") | ||
| withBlock("if (req.subject.#L?.contains(prefix) == true) {", "}", pathMember) { | ||
| write( | ||
| "val updated = req.subject.copy { #L = req.subject.#L?.replace(prefix, #S) }", | ||
| pathMember, | ||
| pathMember, | ||
| "", | ||
| ) | ||
| write("next.call(#T(req.context, updated))", RuntimeTypes.Http.Operation.OperationRequest) | ||
| } | ||
| withBlock("else {", "}") { | ||
| write("next.call(req)") | ||
| } |
There was a problem hiding this comment.
Nit: There's no need to if check here. Attempting to replace a pattern that doesn't match will have no effect:
val prefix = "^/?.*?/"
val updated = req.subject.copy { pathMember = req.subject.pathMember?.replace(prefix, "") }
next.call(OperationRequest(req.context, updated))There was a problem hiding this comment.
You avoid an object copy if you do the check, for what it's worth.
| write("val prefix = #S.toRegex()", "^/?.*?/") | ||
| withBlock("if (req.subject.#L?.contains(prefix) == true) {", "}", pathMember) { | ||
| write( | ||
| "val updated = req.subject.copy { #L = req.subject.#L?.replace(prefix, #S) }", | ||
| pathMember, | ||
| pathMember, | ||
| "", | ||
| ) |
There was a problem hiding this comment.
Style: I personally find it easier to read codegen with fewer placeholder replacements. Consider using triple quotes instead of using #S with string literals:
write("""val prefix = "^/?.*?/".toRegex()""")
withBlock("if (req.subject.#L?.contains(prefix) == true) {", "}", pathMember) {
write(
"""val updated = req.subject.copy { #L = req.subject.#L?.replace(prefix, "") }""",
pathMember,
pathMember,
)| apply GetReusableDelegationSet @httpRequestTests([ | ||
| { | ||
| id: "GetReusableDelegationSetTrimDelegationSetId", | ||
| documentation: "This test validates that delegation set id is correctly trimmed", | ||
| method: "GET", | ||
| protocol: "aws.protocols#restXml", | ||
| uri: "/2013-04-01/delegationset/DELEGATIONSETID", | ||
| bodyMediaType: "application/xml", | ||
| params: { | ||
| "Id": "/delegationset/DELEGATIONSETID" | ||
| } | ||
| }, | ||
| ]) No newline at end of file |
There was a problem hiding this comment.
Question: How/why is delegationset being trimmed in this test?
There was a problem hiding this comment.
- how - the generated regex catches it like anything else
- why - Great question! this gets at the underlying issue that there's no real record of how this customization is meant to work other than how SDKs have implemented it. I learned today that this one in particular is pretty old, it goes back to 2012 in Java v1. Java v2's version basically came from porting over v1. There's apparently no reference for how it's supposed to work outside of how SDKs have written it.
This particular test case is one pulled from rust's codebase.
|
Kudos, SonarCloud Quality Gate passed!
|
|
A new generated diff is ready to view: __generated-main...__generated-feat-route53-trimprefix |
3 participants
Issue #
#509
Description of changes
Trim route53 resource ID prefixes that are generally returned by the service and round-tripped in other requests where the value is URI-bound.
See the added
route53-testsfor input examples.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.