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.

Sign up for GitHub

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

Jump to bottom

Auth Manager API part 4: RESTClient, HTTPClient #11992

Open

adutra wants to merge 2 commits into apache:main from adutra:auth-manager-4

+773 −790

Contributor

adutra commented Jan 17, 2025

4th PR for the Auth Manager API. Previous ones:

This PR introduces the required changes to RESTClient and HTTPClient. It also introduces a BaseHTTPClient abstract class to facilitate the creation and execution of HTTPRequests in a consistent way.

The biggest change in actually in TestRESTCatalog, because almost every Mockito.verify() call needs to be adapted.

The AuthManager API is still "unplugged" at this point.

\cc @nastra @danielcweeks


          Auth Manager API part 4: RESTClient, HTTPClient

81b16de

github-actions bot added the core label

adutra mentioned this pull request

Auth Manager API part 5: SigV4 Auth Manager #11995

Open

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                           any());
                 }
                 @Test
                 public void testCatalogCredentialNoOauth2ServerUri() {
-                  Map<String, String> emptyHeaders = ImmutableMap.of();

Contributor

nastra Jan 20, 2025

nit: we might want to consider keeping this so that it's clearer that empty headers are expected

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                           any());
                 }
                 @ParameterizedTest
                 @ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
                 public void testCatalogCredential(String oauth2ServerUri) {
-                  Map<String, String> emptyHeaders = ImmutableMap.of();

Contributor

nastra Jan 20, 2025

same as above

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                           any());
                 }
                 @ParameterizedTest
                 @ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
                 public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) {
-                  Map<String, String> emptyHeaders = ImmutableMap.of();

Contributor

nastra Jan 20, 2025

same as above

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                             any());
                   }
                 }
                 @ParameterizedTest
                 @ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
                 public void testCatalogTokenRefresh(String oauth2ServerUri) {
-                  Map<String, String> emptyHeaders = ImmutableMap.of();

Contributor

nastra Jan 20, 2025

same as above

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                                     any());
                           });
                 }
                 @ParameterizedTest
                 @ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
                 public void testCatalogRefreshedTokenIsUsed(String oauth2ServerUri) {
-                  Map<String, String> emptyHeaders = ImmutableMap.of();

Contributor

nastra Jan 20, 2025

same as above

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                   // expires at epoch second = 1
                   String token =
                       "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.gQADTbdEv-rpDWKSkGLbmafyB5UUjTdm9B_1izpuZ6E";
-                  Map<String, String> emptyHeaders = ImmutableMap.of();

Contributor

nastra Jan 20, 2025

same here

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                           any());
                 }
                 @ParameterizedTest
                 @ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
                 public void testCatalogTokenRefreshFailsAndUsesCredentialForRefresh(String oauth2ServerUri) {
-                  Map<String, String> emptyHeaders = ImmutableMap.of();

Contributor

nastra Jan 20, 2025

here too

nastra reviewed

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Outdated

                           eq(OAuthTokenResponse.class),
-                          eq(ImmutableMap.of()),
+                          anyMap(),

Contributor

nastra Jan 20, 2025

should this rather verify that this should be an empty map instead of any map?


          review

a513a59

nastra approved these changes

View reviewed changes

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

Labels

core