fix(build): Remove duplicate BasicPrincipal from presto-main-base#27187
Merged
feilong-liu merged 1 commit intoprestodb:masterfrom Feb 23, 2026
Merged
fix(build): Remove duplicate BasicPrincipal from presto-main-base#27187feilong-liu merged 1 commit intoprestodb:masterfrom
feilong-liu merged 1 commit intoprestodb:masterfrom
Conversation
…and presto-internal-communication Summary: OSS PR prestodb#27165 extracted internal communication authentication into a new presto-internal-communication module. It properly moved InternalCommunicationConfig, RoleType, InternalAuthenticationManager, and InternalAuthenticationFilter from their old locations, but missed removing BasicPrincipal.java from presto-main-base. This causes duplicate-finder-maven-plugin failures in CI for presto-configerator, presto-metalake-merger, presto-crypto-functions, and presto-facebook-thrift-testing-server, since both presto-main-base and presto-internal-communication contain com.facebook.presto.security.BasicPrincipal. Fix: Remove BasicPrincipal from presto-main-base and add presto-internal-communication as a dependency (with exclusions for http-server, http-client, jaxrs, jjwt-api, and drift-transport-netty to avoid transitive dependency bloat that would break presto-spark-base's dependency scope check). Reviewed By: abhinavmuk04 Differential Revision: D94005053
|
|
Contributor
Reviewer's GuideRemoves the duplicate BasicPrincipal definition from presto-main-base and instead depends on the shared presto-internal-communication module, while carefully excluding heavy transitive dependencies to keep the dependency graph lean and compatible with existing scope checks. Class diagram for BasicPrincipal relocation to presto-internal-communicationclassDiagram
class presto_main_base {
}
class presto_internal_communication {
}
class BasicPrincipal {
+String name
+BasicPrincipal(String name)
+String getName()
}
presto_internal_communication <|-- BasicPrincipal
presto_main_base ..> BasicPrincipal
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
kevintang2022
approved these changes
Feb 23, 2026
Contributor
|
Thanks for the fix.
Wondering if it should be enabled (can be in a separate PR though). |
feilong-liu
approved these changes
Feb 23, 2026
This was referenced Mar 31, 2026
15 tasks
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.
Description
PR #27165 extracted internal communication authentication into the new presto-internal-communication module. It properly moved InternalCommunicationConfig, RoleType, InternalAuthenticationManager, and
InternalAuthenticationFilter from their old locations but missed removing BasicPrincipal.java from presto-main-base.
This causes com.facebook.presto.security.BasicPrincipal to exist in both presto-main-base and presto-internal-communication, creating duplicate classes on the classpath of any module that transitively depends on
both (e.g., via presto-main).
Motivation and Context
The duplicate-finder-maven-plugin (which is enabled in downstream builds) fails with:
Found duplicate (but equal) classes in [presto-internal-communication:0.297-SNAPSHOT, presto-main-base:0.297-SNAPSHOT]:
com.facebook.presto.security.BasicPrincipal
The OSS build doesn't catch this because the duplicate-finder plugin is skipped for all modules.
Impact
No production or user-facing impact. BasicPrincipal is still available on the classpath from presto-internal-communication. The only usage in presto-main-base is SessionRepresentation.java, which resolves it from
the new dependency.
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.