Core: Simplify AuthManager API #12555

adutra · 2025-03-17T17:01:16Z

The current AuthManager interface exposes various methods for creating sessions: initSession(), catalogSession(), contextualSession(), tableSession(), etc.

Naming its methods after the intended usage of the returned session was, in hindsight, a questionable design choice: in some cases, the right method to call may not be so obvious, and if more session "flavors" are need in the future, more methods would need to be added to the interface.

This PR aims at fixing this by unifying all previous methods under one single method. The increased flexibility of the new method is due to its argument of type AuthScope, which can be freely implemented according to the caller's needs.

core/src/main/java/org/apache/iceberg/rest/auth/AuthManager.java

nastra · 2025-03-25T09:46:34Z

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java

+  @Override
+  @Deprecated
+  @SuppressWarnings("deprecation")
+  public OAuth2Util.AuthSession catalogSession(


shouldn't all of these overrides rather go into AuthManager where the default impl would be to call what's being called here?

We can't remove these methods without introducing revapi exceptions.

If you are OK with revapi exceptions, then I'm fine with that too!

what I mean is this: the AuthManager already deprecated those methods and here we're just overriding them for the sake of providing a slightly different impl. IMO that impl should go into the AuthManager so that we don't need to do anything for those deprecated methods here

If I remove the methods here I get many revapi error like this:

java.method.returnTypeChanged: The return type changed from 'org.apache.iceberg.rest.auth.OAuth2Util.AuthSession' to 'org.apache.iceberg.rest.auth.AuthSession'. old: method org.apache.iceberg.rest.auth.OAuth2Util.AuthSession org.apache.iceberg.rest.auth.OAuth2Manager::catalogSession(org.apache.iceberg.rest.RESTClient, java.util.Map<java.lang.String, java.lang.String>) new: method org.apache.iceberg.rest.auth.AuthSession org.apache.iceberg.rest.auth.AuthManager::catalogSession(org.apache.iceberg.rest.RESTClient, java.util.Map<java.lang.String, java.lang.String>) @ org.apache.iceberg.rest.auth.OAuth2Manager

I tried something in 75acdbc – let me know if that's better.

hm ok I was hoping that RevAPI wouldn't complain. The changes introduced in that commit make sense to me, thanks

nastra · 2025-03-25T09:54:02Z

core/src/test/java/org/apache/iceberg/rest/auth/TestBasicAuthManager.java

@@ -30,7 +30,7 @@ class TestBasicAuthManager {
  @Test
  void missingUsername() {
    try (AuthManager authManager = new BasicAuthManager("test")) {
-      assertThatThrownBy(() -> authManager.catalogSession(null, Map.of()))
+      assertThatThrownBy(() -> authManager.authSession(ImmutableAuthScopes.Standalone.of(Map.of())))


so from how I understand it, the Standalone scope is basically the same as the Catalog scope and only differs in naming?

Mostly, yes. I tried to find a name that would capture its "generic" purpose better than just calling it Catalog or Table.

nastra · 2025-03-25T09:55:52Z

aws/src/main/java/org/apache/iceberg/aws/RESTSigV4AuthManager.java

  @Override
+  @Deprecated
+  @SuppressWarnings("deprecation")


I don't think this is necessary. All the other components that we deprecate don't suppress this warning, since we want to make sure that users see this deprecation and switch to the new API

nastra · 2025-03-25T10:02:04Z

core/src/main/java/org/apache/iceberg/rest/auth/AuthScopes.java

+  @Value.Immutable
+  abstract class Initial implements AuthScope {
+
+    @Value.Parameter(order = 1)


I'm not a big fan of using @Value.Parameter to have an of() method being generated with the correct paramters. In all other places we typically create the of() method ourselves to better control the signature of the method. This also makes it easier for us down the line in case anything changes with the parameters, since we can properly deprecate and create overloading methods when needed. When the of() method is generated, it's more difficult to go through our API deprecation cycle.

That being said, I would change this to

diff --git a/core/src/main/java/org/apache/iceberg/rest/auth/AuthScopes.java b/core/src/main/java/org/apache/iceberg/rest/auth/AuthScopes.java index 4465c95290..c9ab1fc5e3 100644 --- a/core/src/main/java/org/apache/iceberg/rest/auth/AuthScopes.java +++ b/core/src/main/java/org/apache/iceberg/rest/auth/AuthScopes.java @@ -36,7 +36,6 @@ public interface AuthScopes { @Value.Immutable abstract class Initial implements AuthScope { - @Value.Parameter(order = 1) @Override public abstract Map<String, String> properties(); @@ -49,6 +48,10 @@ public interface AuthScopes { public final boolean cacheable() { return false; } + + public static Initial of(Map<String, String> properties) { + return ImmutableAuthScopes.Initial.builder().properties(properties).build(); + } } /** @@ -58,7 +61,6 @@ public interface AuthScopes { @Value.Immutable abstract class Catalog implements AuthScope { - @Value.Parameter(order = 1) @Override public abstract Map<String, String> properties(); @@ -71,6 +73,10 @@ public interface AuthScopes { public final boolean cacheable() { return false; } + + public static Catalog of(Map<String, String> properties) { + return ImmutableAuthScopes.Catalog.builder().properties(properties).build(); + } } /** @@ -80,10 +86,8 @@ public interface AuthScopes { @Value.Immutable abstract class Contextual implements AuthScope { - @Value.Parameter(order = 1) public abstract SessionCatalog.SessionContext context(); - @Value.Parameter(order = 2) @Value.Lazy @Override public Map<String, String> properties() { @@ -94,7 +98,6 @@ public interface AuthScopes { return RESTUtil.merge(properties, credentials); } - @Value.Parameter(order = 3) @Override @Nonnull public abstract AuthSession parent(); @@ -103,6 +106,10 @@ public interface AuthScopes { public final boolean cacheable() { return true; } + + public static Contextual of(SessionCatalog.SessionContext context, AuthSession parent) { + return ImmutableAuthScopes.Contextual.builder().context(context).parent(parent).build(); + } } /** @@ -112,14 +119,11 @@ public interface AuthScopes { @Value.Immutable abstract class Table implements AuthScope { - @Value.Parameter(order = 1) public abstract TableIdentifier identifier(); - @Value.Parameter(order = 2) @Override public abstract Map<String, String> properties(); - @Value.Parameter(order = 3) @Override @Nonnull public abstract AuthSession parent(); @@ -128,6 +132,15 @@ public interface AuthScopes { public final boolean cacheable() { return true; } + + public static Table of( + TableIdentifier identifier, Map<String, String> properties, AuthSession parent) { + return ImmutableAuthScopes.Table.builder() + .identifier(identifier) + .properties(properties) + .parent(parent) + .build(); + } } /** @@ -137,7 +150,6 @@ public interface AuthScopes { @Value.Immutable abstract class Standalone implements AuthScope { - @Value.Parameter(order = 1) @Override public abstract Map<String, String> properties(); @@ -146,5 +158,9 @@ public interface AuthScopes { public boolean cacheable() { return false; } + + public static Standalone of(Map<String, String> properties) { + return ImmutableAuthScopes.Standalone.builder().properties(properties).build(); + } } }

And when a scope is instantiated I would use AuthScopes.Xyz.of(..) instead of ImmutableAuthScopes.Xyz.of(..)

Very nice suggestion, patch applied 👍

nastra · 2025-03-25T10:14:22Z

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java

    this.name = managerName;
  }

  @Override
+  public OAuth2Manager withClient(RESTClient restClient) {
+    Preconditions.checkNotNull(restClient, "invalid restClient: null");


this should probably be a IAE to indicate that the argument passed cannot be null. Also I would update the error msg to Invalid REST client: null

nastra · 2025-03-25T10:14:47Z

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java

+
+  @Override
+  public OAuth2Util.AuthSession authSession(AuthScope scope) {
+    Preconditions.checkNotNull(scope, "invalid scope: null");


Suggested change

Preconditions.checkNotNull(scope, "invalid scope: null");

Preconditions.checkArgument(null != scope, "Invalid auth scope: null");

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java

nastra · 2025-03-25T10:22:47Z

core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java

@@ -130,24 +136,26 @@ void catalogSessionNoOAuth2Properties() {
  @Test
  void catalogSessionTokenProvided() {
    Map<String, String> properties = Map.of(OAuth2Properties.TOKEN, "test");
-    try (OAuth2Manager manager = new OAuth2Manager("test");
-        OAuth2Util.AuthSession catalogSession = manager.catalogSession(client, properties)) {
+    try (OAuth2Manager manager = new OAuth2Manager("test").withClient(client).withClient(client);


Suggested change

try (OAuth2Manager manager = new OAuth2Manager("test").withClient(client).withClient(client);

try (OAuth2Manager manager = new OAuth2Manager("test").withClient(client);

The current `AuthManager` interface exposes various methods for creating sessions: `initSession()`, `catalogSession()`, `contextualSession()`, `tableSession()`, etc. Naming its methods after the intended usage of the returned session was, in hindsight, a questionable design choice: in some cases, the right method to call may not be so obvious, and if more session "flavors" are need in the future, more methods would need to be added to the interface. This PR aims at fixing this by unifying all previous methods under one single method. The increased flexibility of the new method is due to its argument of type `AuthScope`, which can be freely implemented according to the caller's needs.

nastra · 2025-03-26T10:30:39Z

core/src/main/java/org/apache/iceberg/rest/auth/AuthManager.java

  }

  /**
-   * Returns a long-lived session whose lifetime is tied to the owning catalog. This session serves


minor: I don't think it's necessary to remove all javadoc just because we're deprecating something. I would just leave it in, since that also reduces the size of the diff

adutra · 2025-03-28T15:17:01Z

@nastra is there any pending issues in this PR?

github-actions bot added core AWS GCP labels Mar 17, 2025

adutra force-pushed the auth-manager-api-redesign branch 2 times, most recently from 9ff0603 to 5a9aa92 Compare March 18, 2025 00:12

ajantha-bhat modified the milestone: Iceberg 1.9.0 Mar 18, 2025

nastra reviewed Mar 25, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/auth/AuthManager.java Show resolved Hide resolved

nastra reviewed Mar 25, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java Show resolved Hide resolved

nastra reviewed Mar 25, 2025

View reviewed changes

adutra added 6 commits March 26, 2025 10:14

review

65dbc52

apply patch

bbb1924

cleanup after patch

ca21154

review

d26929b

checkstyle

414291e

adutra force-pushed the auth-manager-api-redesign branch from 041bcd0 to 414291e Compare March 26, 2025 09:24

adutra added 2 commits March 26, 2025 10:37

rework deprecated methods

75acdbc

fix test due to rebase conflict

b58df4e

nastra reviewed Mar 26, 2025

View reviewed changes

put javadocs back

e5bdb33

nastra requested a review from danielcweeks March 28, 2025 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Simplify AuthManager API #12555

Core: Simplify AuthManager API #12555

adutra commented Mar 17, 2025

nastra Mar 25, 2025

adutra Mar 25, 2025

nastra Mar 26, 2025

adutra Mar 26, 2025

adutra Mar 26, 2025

nastra Mar 26, 2025

nastra Mar 25, 2025

adutra Mar 25, 2025

nastra Mar 25, 2025

nastra Mar 25, 2025

adutra Mar 25, 2025

nastra Mar 25, 2025

nastra Mar 25, 2025

nastra Mar 25, 2025

nastra Mar 26, 2025

adutra commented Mar 28, 2025

	Preconditions.checkNotNull(scope, "invalid scope: null");
	Preconditions.checkArgument(null != scope, "Invalid auth scope: null");

	try (OAuth2Manager manager = new OAuth2Manager("test").withClient(client).withClient(client);
	try (OAuth2Manager manager = new OAuth2Manager("test").withClient(client);

Core: Simplify AuthManager API #12555

Are you sure you want to change the base?

Core: Simplify AuthManager API #12555

Conversation

adutra commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra commented Mar 28, 2025