HttpSigner interface changes #4018

gosar · 2023-05-16T18:23:12Z

Motivation and Context

Modifications

Make Identity explicit in HttpSignRequest
Add consumer builder pattern to HttpSigner methods
Make explicit sub types for sync and async to make it easier to deal with generics, especially with Publisher. Removes need to use Class directly in methods.

Other fixes:

Remove for HttpAuthOption.Builder.schemeId
Fix generics related warning in Signer interfaces

Usage is shown in HttpSignerTest included in this PR.

Testing

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSigner.java

They have default implementation that rely on new DefaultHttpSignRequest.BuilderImpl constructor that relies on the generic IdentityT without knowing the Class of that type. This class is @SdkInternalApi so this constructor is considered private. Also, make HttpSignRequest.builder take Class<IdentityT> as parameter. Also, make IdentityT type extends Identity in HttpSignRequest.

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSigner.java

core/http-auth-spi/src/test/java/software/amazon/awssdk/http/auth/spi/HttpSignerTest.java

This fixes HttpSignerTest.signAsync_usingRequest_works test.

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/AsyncHttpSignRequest.java

millems · 2023-05-18T19:37:15Z

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSignRequest.java

-    static <T> Builder<T> builder(Class<T> payloadType) {
-        return new DefaultHttpSignRequest.BuilderImpl(payloadType);
-    }
+public interface HttpSignRequest<PayloadT, IdentityT extends Identity> {


Imo, this should implement ToCopyableBuilder and its builder should implement CopyableBuilder. That prevents some issues here, like needing to remember to override every parent method in the child class.

Unfortunately it makes the parameters on this type even crazier.

To make the room, we could change IdentityT identity() to Identity identity() and Optional<PayloadT> payload() to Optional<? extends Object> payload(), and then override them in the child types to return the exact types. Even better, we might just remove the getters from here entirely and rely on the subtype. I don't expect it to be a major problem to need to use the subtype to get access to the payload or identity.

We'd also need to remove the payload and identity setters from this base builder, but that's probably okay - we can just put them in the child types. It should be very rare that someone needs to set the identity or payload using the base builder type...

That prevents some issues here, like needing to remember to override every parent method in the child class.

Without using To/CopyableBuilder, I was able to remove the overriding of methods in the builder sub-interfaces to return the sub-type by adding a generic type parameter for the builder, in these commits de704e7 and 3e24cc4.

I'm not sure if we'll really need to mutate these objects, so I think we can defer making these To/CopyableBuilder until needed?

millems · 2023-05-18T19:38:34Z

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/SignedHttpRequest.java

@@ -58,16 +58,16 @@ static <T> Builder<T> builder(Class<T> payloadType) {
    /**
     * A builder for a {@link SignedHttpRequest}.
     */
-    interface Builder<PayloadT> extends SdkBuilder<Builder<PayloadT>, SignedHttpRequest> {
+    interface Builder<PayloadT> extends SdkBuilder<Builder<PayloadT>, SignedHttpRequest<PayloadT>> {


Same comment here. CopyableBuilder?

...-spi/src/main/java/software/amazon/awssdk/http/auth/spi/internal/DefaultHttpSignRequest.java

And removed `payloadType()` accessor. Also, HttpSignRequest builders take Identity as parameter instead of Class<IdentityT>.

* Added missing Builder method override for identity * Fixed javadocs * Fixed toString

millems · 2023-05-22T20:23:08Z

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSignRequest.java

    Optional<PayloadT> payload();

    /**
-     * Returns the property that the {@link HttpSigner} can use during signing.
+     * Returns the identity.
+     */
+    IdentityT identity();


Imo, I'd rather not parameterize this types and just do Optional<?> payload() and Identity identity() here. Subtypes would override them with the more-specific return type.

This makes it so that code that deal with the base sign request can just deal with a SignRequest instead of a SignRequest<?, ?>, and the base builder can just deal with a Builder<?> instead of a Builder<?, ?, ?>.

WDYT?

I think code that deals with base sign request, might benefit from the type, atleast the IdentityT. So the BearerTokenSigner can extract the token from HttpSignRequest<?, TokenIdentity identity>. Though I can see removing the PayloadT (will reduce it by one), but means the subtypes have to (re)define the payload() method instead of specifying the generic in type definition. And as an interface, having a HttpSignRequest that has a payload() accessor but doesn't know it's type doesn't feel great to me, so I'm in favor of keeping them.

Cool. And we can reassess at the surface area review as well, once we can see the impact on the signers, etc.

millems · 2023-05-22T20:24:13Z

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSignRequest.java

        /**
         * Set the body payload of the request. A payload is optional. By default, the payload will be empty.
         */
-        Builder payload(PayloadT payload);
+        B payload(PayloadT payload);
+
+        /**
+         * Set the identity of the request.
+         */
+        B identity(IdentityT identity);


If we go with the suggestion I have above, we'd remove these from the base builder and put it in the subtypes.

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSigner.java

millems

A minor nit, otherwise lgtm.

millems · 2023-05-24T14:48:58Z

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSignRequest.java

 */
-@SdkPublicApi
+@SdkProtectedApi


This is still a public api.

millems · 2023-05-24T14:50:05Z

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSignRequest.java

    Optional<PayloadT> payload();

    /**
-     * Returns the property that the {@link HttpSigner} can use during signing.
+     * Returns the identity.
+     */
+    IdentityT identity();


Cool. And we can reassess at the surface area review as well, once we can see the impact on the signers, etc.

sonarqubecloud · 2023-05-24T19:36:54Z

SonarCloud Quality Gate failed.

4 Bugs
0 Vulnerabilities
0 Security Hotspots
103 Code Smells

65.3% Coverage
1.0% Duplication

gosar added 4 commits May 15, 2023 22:47

Fix generics related warning in Signer interfaces

2fffa5a

Remove <T> for HttpAuthOption.Builder.schemeId

f1cf80f

Remove wildcards from sign methods

0690ad8

Make Identity explicit in HttpSignRequest

daaf834

gosar requested a review from a team as a code owner May 16, 2023 18:23

gosar mentioned this pull request May 16, 2023

HttpSigner interface changes #4017

Closed

12 tasks

gosar commented May 16, 2023

View reviewed changes

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSigner.java Outdated Show resolved Hide resolved

gosar added 3 commits May 16, 2023 12:28

Make IdentityT type extend Identity in HttpSigner

d9c7ac9

Add test to show HttpSigner usage

33a9b04

gosar commented May 17, 2023

View reviewed changes

core/http-auth-spi/src/main/java/software/amazon/awssdk/http/auth/spi/HttpSigner.java Outdated Show resolved Hide resolved

gosar commented May 17, 2023

View reviewed changes

core/http-auth-spi/src/test/java/software/amazon/awssdk/http/auth/spi/HttpSignerTest.java Outdated Show resolved Hide resolved

gosar added 3 commits May 17, 2023 13:39

Note identityType parameter is ignored in HttpSignRequest.builder

fc79989

Use separate interfaces for sync and async sign requests

e1cc7bc

This fixes HttpSignerTest.signAsync_usingRequest_works test.

Use abstract DefaultHttpSignRequest to avoid duplication

74e085b

gosar requested a review from millems May 18, 2023 18:15

millems reviewed May 18, 2023

View reviewed changes

gosar added 5 commits May 19, 2023 14:15

Split sync/async interfaces for SignedHttpRequest

80bc11c

And removed `payloadType()` accessor. Also, HttpSignRequest builders take Identity as parameter instead of Class<IdentityT>.

Minor fixes

ab7fdd8

* Added missing Builder method override for identity * Fixed javadocs * Fixed toString

Make protected properties Map an unmodifiableMap

a6b7f64

Remove overriding in HttpSignRequest.Builder sub-interfaces

de704e7

Remove overriding in SignedHttpRequest.Builder sub-interfaces

3e24cc4

millems reviewed May 22, 2023

View reviewed changes

gosar added 2 commits May 22, 2023 16:04

Allow sign methods take request with subtype of the IdentityT

8d5ee7e

Make HttpSignRequest @SdkProtectedApi

a8ef98f

millems approved these changes May 24, 2023

View reviewed changes

Make HttpSignRequest @SdkPublicApi

6f9d3d1

gosar merged commit 28ddc60 into feature/master/sra-identity-auth May 24, 2023

gosar deleted the gosar/sra-ia-signer-interface-updates-2 branch May 24, 2023 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HttpSigner interface changes #4018

HttpSigner interface changes #4018

Uh oh!

gosar commented May 16, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

millems May 18, 2023 •

edited

Loading

Uh oh!

gosar May 20, 2023

Uh oh!

millems May 18, 2023

Uh oh!

Uh oh!

millems May 22, 2023

Uh oh!

gosar May 22, 2023

Uh oh!

millems May 24, 2023

Uh oh!

millems May 22, 2023

Uh oh!

Uh oh!

millems left a comment

Uh oh!

millems May 24, 2023

Uh oh!

millems May 24, 2023

Uh oh!

sonarqubecloud bot commented May 24, 2023

Uh oh!

Uh oh!

HttpSigner interface changes #4018

HttpSigner interface changes #4018

Uh oh!

Conversation

gosar commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

millems May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

millems left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 24, 2023

Uh oh!

Uh oh!

gosar commented May 16, 2023 •

edited

Loading

millems May 18, 2023 •

edited

Loading