-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add Gzip request compression feature #467
Conversation
...n/src/main/java/software/amazon/smithy/go/codegen/requestcompression/RequestCompression.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/software/amazon/smithy/go/codegen/requestcompression/RequestCompression.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/software/amazon/smithy/go/codegen/requestcompression/RequestCompression.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/software/amazon/smithy/go/codegen/requestcompression/RequestCompression.java
Outdated
Show resolved
Hide resolved
This needs a changelog entry, the tool we use works exactly as it does in the SDK. |
return out, metadata, fmt.Errorf("failed to set request stream, %v", err) | ||
} | ||
*req = *newReq | ||
req.Header.Add("Content-Encoding", algorithm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have to append if there's an existing header?
i.e. (pseudocode)
if (content-encoding exists on request)
existing encoding header += ', algorithm'
else
set header directly to algorithm
"add", ADD_REQUEST_COMPRESSION, | ||
"stack", stackSymbol, | ||
"addInternal", addInternalSymbol, | ||
"algorithms", String.format("\"%s\"", String.join(",", algorithms)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason to microformat the list of algorithms like this, just generate it directly, e.g.
@requestCompression(
encodings: ["gzip", "br"]
)
translates to list
[]string{"gzip", "br"}
} | ||
|
||
Set<String> algorithms = RequestCompressionTrait.SUPPORTED_COMPRESSION_ALGORITHMS; | ||
goDelegator.useShapeWriter(service, writeMiddlewareHelper(algorithms)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to generate this per-operation since the compression trait is applied there.
e.g. consider the following three modeled operations and their expected codegen:
@requestCompression{
encodings: ["gzip"]
}
operation Foo {
// ...
}
// should generate:
func addOperationFooRequestCompressionMiddleware(...) {
return stack.Serialize.Add(&requestCompression{
// ...
compressAlgorithms: []string{"gzip"},
}, middleware.After)
}
// and the operation stack setup should now include:
func (c *Client) addOperationFooMiddlewares(...) {
// ...
if err = addOperationFooRequestCompressionMiddleware(...); err != nil {
return err
}
// ...
}
@requestCompression{
encodings: ["br", "gzip"]
}
operation Bar {
// ...
}
// should generate:
func addOperationBarRequestCompressionMiddleware(...) {
return stack.Serialize.Add(&requestCompression{
// ...
compressAlgorithms: []string{"br", "gzip"},
}, middleware.After)
}
// and the operation stack setup should now include:
func (c *Client) addOperationBarMiddlewares(...) {
// ...
if err = addOperationBarRequestCompressionMiddleware(...); err != nil {
return err
}
// ...
}
// no request compression
operation Bar {
// ...
}
// should not generate a middleware, and the operation stack setup shouldn't have anything new
See AwsHttpChecksumGenerator.java
for an example of how this is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I previously thought the SUPPORTED_COMPRESSION_ALGORITHMS
of that trait is static so service level middleware is enough, but seems like we have multi-encodings for diff ops, I will change that
} | ||
|
||
// AddRequestCompression add requestCompression middleware to op stack | ||
func AddRequestCompression(stack *middleware.Stack, DisableRequestCompression bool, RequestMinCompressSizeBytes int64, algorithms string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lowercase the param names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also it's in-context so e.g. DisableRequestCompression
can become simply disabled
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I prefer to keep the naming in middleware fields in case of extra comment
|
||
private GoWriter.Writable writeMiddlewareHelper(SymbolProvider symbolProvider, OperationShape operation) { | ||
var stackSymbol = SymbolUtils | ||
.createPointableSymbolBuilder("Stack", SmithyGoDependency.SMITHY_MIDDLEWARE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SmithyGoTypes
was merged with the SRA auth refactor - you can convert wrote non-generated imports like this to SmithyGoTypes.Middleware.Stack
etc.
(add static symbols to SmithyGoTypes
as needed)
} | ||
return o.equals(operation); | ||
}).registerMiddleware(MiddlewareRegistrar.builder() | ||
.resolvedFunction(SymbolUtils.createValueSymbolBuilder(funcName).build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymbolUtils.buildPackageSymbol(funcName)
} | ||
|
||
private GoWriter.Writable writeMiddlewareHelper(SymbolProvider symbolProvider, OperationShape operation) { | ||
var addInternalSymbol = SymbolUtils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SmithyGoTypes.Private.RequestCompression.AddRequestCompression
(you'll have to create it)
RequestCompressionTrait trait = operation.expectTrait(RequestCompressionTrait.class); | ||
|
||
// build encoding list symbol | ||
StringBuilder algorithmList = new StringBuilder("[]string{"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're overcomplicating this - remember we can use $W
to compose Writables in templates.
Something like the following:
private GoWriter.Writable generateAlgorithmList(List<String> algorithms) {
return goTemplate("""
[]string{
$W
}
""",
GoWriter.ChainWritable.of( // compose a list of writables into a single one
algorithms.stream()
.map(it -> goTemplate("$S,", it))
.toList()
).compose(false)); // compose(false) = don't write newlines between writables
}
then use that in the main template here:
$algorithms:W
// in your map
"algorithms", generateAlgorithmList(algorithms)
for (String algo : trait.getEncodings()) { | ||
algorithmList.append(String.format("\"%s\", ", algo)); | ||
} | ||
String algorithms = algorithmList.substring(0, algorithmList.length() - 2) + "}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be moot when you do the above but this substringing isn't necessary - gofmt
handles the trailing comma in a single-line list
.configFields(ListUtils.of( | ||
ConfigField.builder() | ||
.name(DISABLE_REQUEST_COMPRESSION) | ||
.type(SymbolUtils.createValueSymbolBuilder("bool") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GoUniverseTypes.bool
?
.build(), | ||
ConfigField.builder() | ||
.name(REQUEST_MIN_COMPRESSION_SIZE_BYTES) | ||
.type(SymbolUtils.createValueSymbolBuilder("int64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GoUniverseTypes.Int64
?
*req = *newReq | ||
|
||
if val := req.Header.Get("Content-Encoding"); val != "" { | ||
req.Header.Set("Content-Encoding", val+", "+algorithm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer fmt.Sprintf
for compositions like this:
fmt.Sprintf("%s, %s", val, algorithm)
return next.HandleSerialize(ctx, in) | ||
} | ||
|
||
func gzipCompress(input io.Reader) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but I'd give this it's own file in this package eg private/requestcompression/gzip.go
public static final GoDependency SMITHY_REQUEST_COMPRESSION = | ||
smithy("private/requestcompression", "smithyrequestcompression"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix formatting for this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smithy go doesn't allow single line exceeding 120 chars, that's why there's a separate line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final GoDependency SMITHY_REQUEST_COMPRESSION = | |
smithy("private/requestcompression", "smithyrequestcompression"); | |
public static final GoDependency SMITHY_REQUEST_COMPRESSION = | |
smithy("private/requestcompression", "smithyrequestcompression"); |
...n/src/main/java/software/amazon/smithy/go/codegen/requestcompression/RequestCompression.java
Outdated
Show resolved
Hide resolved
|
||
private static final String REQUEST_MIN_COMPRESSION_SIZE_BYTES = "RequestMinCompressSizeBytes"; | ||
|
||
private final List<RuntimeClientPlugin> runtimeClientPlugins = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping state in an integration is generally not best practice.
...n/src/main/java/software/amazon/smithy/go/codegen/requestcompression/RequestCompression.java
Show resolved
Hide resolved
import software.amazon.smithy.utils.MapUtils; | ||
|
||
|
||
public final class RequestCompression implements GoIntegration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit overall: private fields should go below public fields, and private methods should go towards the bottom of the class below all the public methods.
|
||
@Override | ||
public List<RuntimeClientPlugin> getClientPlugins() { | ||
runtimeClientPlugins.add( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use List.of()
instead, and register all client plugins here immutably instead of using state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI he's doing this because of having to add per-operation middlewares elsewhere in processFinalizedModel
. That's the only way I've ever seen it done unless there's something I'm not considering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getClientPlugins
should probably accept the Model
and service shape ID much like these other functions do long-term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have confirmed that in resolved comments, thx for clarification
@@ -12,3 +12,5 @@ software.amazon.smithy.go.codegen.endpoints.EndpointClientPluginsGenerator | |||
# modeled auth schemes | |||
software.amazon.smithy.go.codegen.integration.auth.SigV4AuthScheme | |||
software.amazon.smithy.go.codegen.integration.auth.AnonymousAuthScheme | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove extra line
This reverts commit 690fcaa.
Add Gzip request compression middleware and codegen logic in smithy go