[#10960] feat(authn): wire built-in IdP storage#11167
Closed
lasdf1234 wants to merge 3 commits into
Closed
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds built-in IdP (user/group) relational persistence and SPI-based loading so core can delegate IdP operations to the idp-basic plugin, including SQL providers for multiple JDBC backends and associated tests.
Changes:
- Introduced IdP user/group POs, MyBatis mappers, and SQL provider factories with backend-specific implementations (MySQL/H2/PostgreSQL).
- Added core-side SPI interfaces, service facades, and a ServiceLoader-based provider loader for IdP metadata operations.
- Wired IdP support into core environment lifecycle and legacy GC paths, plus extensive new unit tests and service registrations.
Reviewed changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/po/TestIdpUserPO.java | Adds unit tests for IdP user PO builder/validation/equality. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/po/TestIdpGroupUserRelPO.java | Adds unit tests for group-user relation PO builder/validation/equality. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/po/TestIdpGroupPO.java | Adds unit tests for IdP group PO builder/validation/equality. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/TestIdpUserMetaPostgreSQLProvider.java | Adds PostgreSQL-specific SQL expectations for IdP user meta provider. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/TestIdpGroupUserRelPostgreSQLProvider.java | Adds PostgreSQL-specific SQL expectations for group-user relation provider. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/TestIdpGroupMetaPostgreSQLProvider.java | Adds PostgreSQL-specific SQL expectations for IdP group meta provider. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/h2/TestIdpUserMetaH2Provider.java | Adds H2 provider coverage via base SQL provider tests. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/h2/TestIdpGroupUserRelH2Provider.java | Adds H2 provider coverage via base SQL provider tests. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/h2/TestIdpGroupMetaH2Provider.java | Adds H2 provider coverage via base SQL provider tests. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestIdpUserMetaBaseSQLProvider.java | Adds base SQL provider tests for IdP user meta queries/mutations. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestIdpGroupUserRelBaseSQLProvider.java | Adds base SQL provider tests for group-user relation SQL (including empty/null list handling). |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestIdpGroupMetaBaseSQLProvider.java | Adds base SQL provider tests for IdP group meta queries/mutations. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/TestIdpBasicMapperPackageProvider.java | Tests mapper package provider returns expected mapper classes. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/TestIdpUserMetaMySQLProvider.java | Adds MySQL provider coverage via base SQL provider tests. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/TestIdpGroupUserRelMySQLProvider.java | Adds MySQL provider coverage via base SQL provider tests. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/TestIdpGroupMetaMySQLProvider.java | Adds MySQL provider coverage via base SQL provider tests. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/provider/TestIdpBasicUserMetaProvider.java | Tests singleton/type bridge and ServiceLoader registration for IdP user provider. |
| plugins/idp-basic/src/test/java/org/apache/gravitino/storage/provider/TestIdpBasicGroupMetaProvider.java | Tests singleton/type bridge and ServiceLoader registration for IdP group provider. |
| plugins/idp-basic/src/main/resources/META-INF/services/org.apache.gravitino.storage.relational.provider.IdpUserMetaProvider | Registers idp-basic IdP user provider for SPI loading. |
| plugins/idp-basic/src/main/resources/META-INF/services/org.apache.gravitino.storage.relational.provider.IdpGroupMetaProvider | Registers idp-basic IdP group provider for SPI loading. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/po/IdpUserPO.java | Adds IdP user persistence object with builder + validation. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/po/IdpGroupUserRelPO.java | Adds group-user relation persistence object with builder + validation. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/po/IdpGroupPO.java | Adds IdP group persistence object with builder + validation. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/IdpUserMetaPostgreSQLProvider.java | PostgreSQL overrides for soft delete timestamp and legacy deletion SQL. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/IdpGroupUserRelPostgreSQLProvider.java | PostgreSQL overrides for relation soft delete timestamp and legacy deletion SQL. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/IdpGroupMetaPostgreSQLProvider.java | PostgreSQL overrides for group soft delete timestamp and legacy deletion SQL. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/IdpUserMetaH2Provider.java | H2 provider adapter extending base SQL provider. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/IdpGroupUserRelH2Provider.java | H2 provider adapter extending base SQL provider. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/h2/IdpGroupMetaH2Provider.java | H2 provider adapter extending base SQL provider. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpUserMetaBaseSQLProvider.java | Adds base SQL templates for IdP user meta operations. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpGroupUserRelBaseSQLProvider.java | Adds base SQL templates for group-user relation operations (with IN-list guarding). |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpGroupMetaBaseSQLProvider.java | Adds base SQL templates for IdP group meta operations. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/IdpBasicMapperPackageProvider.java | Exposes plugin mapper classes to core mapper scanning. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java | Adds backend-based SQL provider selection for IdP user meta. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpUserMetaMapper.java | Adds MyBatis mapper for IdP user meta operations. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpGroupUserRelSQLProviderFactory.java | Adds backend-based SQL provider selection for group-user relations. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpGroupUserRelMapper.java | Adds MyBatis mapper for group-user relation operations. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpGroupMetaSQLProviderFactory.java | Adds backend-based SQL provider selection for IdP group meta. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpGroupMetaMapper.java | Adds MyBatis mapper for IdP group meta operations. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/provider/IdpUserMetaProvider.java | Adds plugin-side compatibility bridge for core IdP user provider SPI. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/provider/IdpGroupMetaProvider.java | Adds plugin-side compatibility bridge for core IdP group provider SPI. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/provider/IdpBasicUserMetaProvider.java | Implements IdP user provider backed by MyBatis mappers. |
| plugins/idp-basic/src/main/java/org/apache/gravitino/storage/provider/IdpBasicGroupMetaProvider.java | Implements IdP group provider backed by MyBatis mappers. |
| core/src/test/java/org/apache/gravitino/storage/relational/service/TestIdpUserMetaService.java | Tests IdP user service delegation and ServiceLoader error cases. |
| core/src/test/java/org/apache/gravitino/storage/relational/service/TestIdpGroupMetaService.java | Tests IdP group service delegation and ServiceLoader error cases. |
| core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java | Extends legacy record existence checks to IdP tables. |
| core/src/test/java/org/apache/gravitino/storage/relational/BackendTestExtension.java | Adds metrics sliding window config stubbing required by backend startup. |
| core/src/test/java/org/apache/gravitino/TestIdpManagerFactory.java | Tests IdP manager SPI loading behavior and error handling. |
| core/src/test/java/org/apache/gravitino/TestGravitinoEnv.java | Tests env initialization/shutdown behavior for IdP manager integration. |
| core/src/main/java/org/apache/gravitino/storage/relational/service/IdpUserMetaService.java | Adds core facade delegating IdP user operations to plugin provider. |
| core/src/main/java/org/apache/gravitino/storage/relational/service/IdpGroupMetaService.java | Adds core facade delegating IdP group operations to plugin provider. |
| core/src/main/java/org/apache/gravitino/storage/relational/provider/IdpUserMetaProvider.java | Adds core SPI for IdP user operations. |
| core/src/main/java/org/apache/gravitino/storage/relational/provider/IdpMetaProviderLoader.java | Adds single-implementation ServiceLoader helper for IdP providers. |
| core/src/main/java/org/apache/gravitino/storage/relational/provider/IdpGroupMetaProvider.java | Adds core SPI for IdP group + relation operations. |
| core/src/main/java/org/apache/gravitino/storage/relational/po/IdpUserMeta.java | Adds core model interface for IdP user meta. |
| core/src/main/java/org/apache/gravitino/storage/relational/po/IdpGroupUserRelMeta.java | Adds core model interface for group-user relation meta. |
| core/src/main/java/org/apache/gravitino/storage/relational/po/IdpGroupMeta.java | Adds core model interface for IdP group meta. |
| core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java | Wires IdP entity types into hard delete legacy data paths. |
| core/src/main/java/org/apache/gravitino/authorization/IdpManager.java | Introduces IdP manager API for user/group management in core. |
| core/src/main/java/org/apache/gravitino/IdpManagerFactory.java | Adds ServiceLoader-based factory for IdP manager SPI. |
| core/src/main/java/org/apache/gravitino/GravitinoEnv.java | Integrates IdP manager lifecycle into core env init/shutdown. |
| core/src/main/java/org/apache/gravitino/Entity.java | Adds new entity types for IDP_USER and IDP_GROUP. |
| core/build.gradle.kts | Adds idp-basic plugin as test runtime dependency for core tests. |
Comments suppressed due to low confidence (2)
plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpUserMetaBaseSQLProvider.java:1
- This SQL script can render invalid SQL when
userNamesis empty (e.g.,IN ()) and may fail ifuserNamesis null. The group-user-rel providers in this PR handle empty/null lists via<choose>andAND 1 = 0;selectIdpUsersshould apply the same pattern (or otherwise guarantee a valid predicate) to prevent runtime SQL errors if callers bypass the current Java-side empty-list guard.
plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/IdpUserMetaPostgreSQLProvider.java:1 - This override ignores the
deletedAtparameter and always uses the database clock. That creates cross-backend semantic differences (MySQL/H2 use the provided parameter) and can surprise API consumers that passdeletedAtexpecting it to be persisted. If using DB time is required for PostgreSQL, consider documenting this explicitly (e.g., Javadoc on the override and/or on the SPI/mapper method) and suppressing the unused-parameter warning; alternatively, align behavior across backends by using#{deletedAt}everywhere.
Comment on lines
+32
to
+49
| public static <T> T loadService(Class<T> serviceClass) { | ||
| List<T> services = new ArrayList<>(); | ||
| for (T service : ServiceLoader.load(serviceClass)) { | ||
| services.add(service); | ||
| } | ||
|
|
||
| if (services.isEmpty()) { | ||
| throw new IllegalStateException( | ||
| String.format("No %s implementation found", serviceClass.getSimpleName())); | ||
| } | ||
|
|
||
| if (services.size() > 1) { | ||
| throw new IllegalStateException( | ||
| String.format("Multiple %s implementations found", serviceClass.getSimpleName())); | ||
| } | ||
|
|
||
| return services.get(0); | ||
| } |
Comment on lines
+463
to
+467
| /** Get the built-in IdP manager implementation. */ | ||
| public IdpManager idpManager() { | ||
| Preconditions.checkArgument(idpManager != null, "GravitinoEnv is not initialized."); | ||
| return idpManager; | ||
| } |
| try { | ||
| idpManager.close(); | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to close IdpManager", e); |
Comment on lines
+45
to
+62
| private static <T> T loadService(Class<T> serviceClass) { | ||
| List<T> services = new ArrayList<>(); | ||
| for (T service : ServiceLoader.load(serviceClass)) { | ||
| services.add(service); | ||
| } | ||
|
|
||
| if (services.isEmpty()) { | ||
| throw new IllegalStateException( | ||
| String.format("No %s implementation found", serviceClass.getSimpleName())); | ||
| } | ||
|
|
||
| if (services.size() > 1) { | ||
| throw new IllegalStateException( | ||
| String.format("Multiple %s implementations found", serviceClass.getSimpleName())); | ||
| } | ||
|
|
||
| return services.get(0); | ||
| } |
| .getDatabaseId(); | ||
|
|
||
| JDBCBackendType jdbcBackendType = JDBCBackendType.fromString(databaseId); | ||
| return IDP_USER_META_SQL_PROVIDER_MAP.get(jdbcBackendType); |
Collaborator
Author
|
Closing per maintainer request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR wires built-in IdP storage into Gravitino runtime on top of
main, including:IdpUserMetaService,IdpGroupMetaService) and JDBC backend integrationIdpManagerFactory/IdpManagerandGravitinoEnvlifecycle wiringidp-basicplugin providers and SPI registrationRebased onto latest
apache/gravitinomain(merged with-X theirsso upstream changes take precedence on conflicts).Part of #10960 / subtask #11044
Why are the changes needed?
The relational storage classes and plugin mappers landed on
mainvia #11127 and #11140, but runtime wiring (env initialization, JDBC backend delegation, provider loading) still needs to land so the built-in IdP storage is usable.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Made with Cursor