fix(sdk)!: enforce sane visibility#371
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an ArchUnit-based PublicApiSurfaceTest, centralizes JUnit Platform test versions and adds ArchUnit dependency, changes InvalidZipException to extend SDKException, makes many internal classes package-private, exposes Autoconfigure.AttributeValueFQN, and promotes TDF to the public API root. ChangesAPI Surface Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SDK's public API surface by adjusting the visibility of numerous classes and members to ensure only intended components are exposed. Key changes include transitioning several classes to package-private visibility, updating InvalidZipException to extend SDKException, and adding a new ArchUnit test, PublicApiSurfaceTest, to enforce reachability-based visibility rules. Review feedback focuses on optimizing the new test by removing an unused import, using more efficient data structures, and eliminating redundant logic in the API reachability algorithm.
| import com.tngtech.archunit.core.importer.ClassFileImporter; | ||
| import com.tngtech.archunit.core.importer.ImportOption; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.BeforeEach; |
There was a problem hiding this comment.
The import org.junit.jupiter.api.BeforeEach is unused and should be removed to keep the code clean.
| Config.class.getName(), | ||
| RequestHelper.class.getName() | ||
| ); | ||
| static List<JavaClass> reachableClasses; |
There was a problem hiding this comment.
Consider using a Set<JavaClass> for reachableClasses instead of a List. This would improve the performance of the contains checks in the test methods from O(N) to O(1).
| static List<JavaClass> reachableClasses; | |
| static Set<JavaClass> reachableClasses; |
There was a problem hiding this comment.
this isn't java.lang.reflect
| for (JavaType r: m.getParameterTypes()) { | ||
| addAllRawTypes(r, work, queued); | ||
| } | ||
| m.getParameterTypes().forEach(t -> addAllRawTypes(t, work, queued)); |
There was a problem hiding this comment.
The loop and the forEach call are performing the exact same operation on m.getParameterTypes(). One of them should be removed to avoid redundancy.
m.getParameterTypes().forEach(t -> addAllRawTypes(t, work, queued));| } | ||
| for (JavaConstructor ctor : c.getConstructors()) { | ||
| if (!isExposed(ctor.getModifiers())) continue; | ||
| addAllRawTypes(ctor.getRawReturnType(), work, queued); |
There was a problem hiding this comment.
Calling addAllRawTypes on the constructor's return type is redundant because JavaConstructor.getRawReturnType() returns the declaring class c, which is already being processed in the current iteration.
X-Test Failure Report |
X-Test Failure Report✅ java@v0.15.0-v0.15.0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/main/java/io/opentdf/platform/sdk/InvalidZipException.java (1)
4-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale Javadoc to match the new exception hierarchy.
The class comment still states
RuntimeException, but Line 8 now extendsSDKException.Proposed patch
/** * InvalidZipException is thrown to indicate that a ZIP file being read - * is invalid or corrupted in some way. This exception extends RuntimeException, - * allowing it to be thrown during the normal operation of the Java Virtual Machine. + * is invalid or corrupted in some way. This exception extends SDKException. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/io/opentdf/platform/sdk/InvalidZipException.java` around lines 4 - 6, The class Javadoc for InvalidZipException is outdated (mentions RuntimeException) but the class now extends SDKException; update the Javadoc for InvalidZipException to state it extends SDKException and accurately describe its role in the new exception hierarchy, referencing InvalidZipException and SDKException so readers know this is a specialized SDKException thrown when a ZIP file is invalid or corrupted.
🧹 Nitpick comments (1)
sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java (1)
63-102: 💤 Low valueMinor: inconsistent access modifier on test methods.
Line 64 declares
void onlyReachableTypesAreExposed()without an access modifier, while line 85 declarespublic void allReachableTypesAreExposed(). For consistency, both should omit thepublicmodifier (JUnit 5 doesn't require it).Suggested fix
`@Test` -public void allReachableTypesAreExposed() { +void allReachableTypesAreExposed() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java` around lines 63 - 102, In PublicApiSurfaceTest, make the access modifiers consistent by removing the unnecessary public modifier from the allReachableTypesAreExposed() test method so it matches onlyReachableTypesAreExposed() (both package-private), i.e., change the signature of allReachableTypesAreExposed() to have no access modifier; leave the method body and annotations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/Autoconfigure.java`:
- Line 57: The public nested type AttributeValueFQN is inaccessible because its
enclosing class Autoconfigure is package-private; update the visibility so
external callers of Config.TDFConfig.attributes, Config.withDataAttributes,
Config.withDataAttributeValues, and SDK.attributeValueExists can reference it:
either make Autoconfigure public or extract AttributeValueFQN into its own
public top-level class (public class AttributeValueFQN) and update any
references accordingly to preserve API compatibility.
---
Outside diff comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/InvalidZipException.java`:
- Around line 4-6: The class Javadoc for InvalidZipException is outdated
(mentions RuntimeException) but the class now extends SDKException; update the
Javadoc for InvalidZipException to state it extends SDKException and accurately
describe its role in the new exception hierarchy, referencing
InvalidZipException and SDKException so readers know this is a specialized
SDKException thrown when a ZIP file is invalid or corrupted.
---
Nitpick comments:
In `@sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java`:
- Around line 63-102: In PublicApiSurfaceTest, make the access modifiers
consistent by removing the unnecessary public modifier from the
allReachableTypesAreExposed() test method so it matches
onlyReachableTypesAreExposed() (both package-private), i.e., change the
signature of allReachableTypesAreExposed() to have no access modifier; leave the
method body and annotations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78021bcb-1ccf-441c-af28-c172effae882
📒 Files selected for processing (21)
sdk/pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/AesGcm.javasdk/src/main/java/io/opentdf/platform/sdk/AssertionConfig.javasdk/src/main/java/io/opentdf/platform/sdk/AsymDecryption.javasdk/src/main/java/io/opentdf/platform/sdk/AsymEncryption.javasdk/src/main/java/io/opentdf/platform/sdk/Autoconfigure.javasdk/src/main/java/io/opentdf/platform/sdk/CryptoUtils.javasdk/src/main/java/io/opentdf/platform/sdk/ECCurve.javasdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javasdk/src/main/java/io/opentdf/platform/sdk/EntityIdentifiers.javasdk/src/main/java/io/opentdf/platform/sdk/InvalidZipException.javasdk/src/main/java/io/opentdf/platform/sdk/Manifest.javasdk/src/main/java/io/opentdf/platform/sdk/Planner.javasdk/src/main/java/io/opentdf/platform/sdk/PolicyObject.javasdk/src/main/java/io/opentdf/platform/sdk/Resources.javasdk/src/main/java/io/opentdf/platform/sdk/TDFReader.javasdk/src/main/java/io/opentdf/platform/sdk/TDFWriter.javasdk/src/main/java/io/opentdf/platform/sdk/ZipReader.javasdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.javasdk/src/test/java/io/opentdf/platform/sdk/Fuzzing.javasdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java
X-Test Failure Report |
X-Test Failure Report |
…force-sane-visibility
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java (1)
43-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
TDFto the designated roots if this PR intends it to be a supported entry point.
TDFbecomes public in this change set, but this test still seeds reachability only fromSDK,SDKBuilder,AssertionConfig,Config,RequestHelper,PolicyEnums, andEntityIdentifiers. That means the ArchUnit rule validatesTDFonly if some other root happens to reference it, instead of protecting it as an intentional public root.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java` around lines 43 - 50, The API roots set in PublicApiSurfaceTest omits TDF even though TDF was made public; update the API_ROOTS constant used by the ArchUnit test to include TDF.class.getName() so TDF is treated as an explicit public entry point (modify the Set.of(...) in the API_ROOTS declaration that currently lists SDK, SDKBuilder, AssertionConfig, Config, RequestHelper, PolicyEnums, EntityIdentifiers to also include TDF.class.getName()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/TDF.java`:
- Line 34: TDF is declared public but its constructors and entry-point factory
methods are package-private, making the type unusable to external callers; fix
by making the TDF constructors and all factory/entry methods (notably createTDF
and loadTDF and any overloads) public so external consumers can instantiate/use
the class, and ensure any other package-private methods in the TDF class that
are intended as part of the public SDK surface are likewise promoted to public
(adjust visibility for constructors and the methods referenced in the TDF class
accordingly).
In `@sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java`:
- Around line 119-121: The superclass lambda currently ignores the discovered
superclass value `ec` and re-enqueues `c`, so change the lambda to operate on
`ec`: call addAllRawTypes(ec, work, queued) (instead of addAllRawTypes(c,...))
and also use the superclass's interfaces by replacing the external
work.addAll(c.getRawInterfaces()) with work.addAll(ec.getRawInterfaces()) inside
the ifPresent block (or otherwise ensure you call getRawInterfaces() on `ec` not
`c`) so base classes become reachable through subclass exposure.
---
Outside diff comments:
In `@sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java`:
- Around line 43-50: The API roots set in PublicApiSurfaceTest omits TDF even
though TDF was made public; update the API_ROOTS constant used by the ArchUnit
test to include TDF.class.getName() so TDF is treated as an explicit public
entry point (modify the Set.of(...) in the API_ROOTS declaration that currently
lists SDK, SDKBuilder, AssertionConfig, Config, RequestHelper, PolicyEnums,
EntityIdentifiers to also include TDF.class.getName()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bae3c51a-a659-463b-a63d-4d5305550333
📒 Files selected for processing (3)
sdk/src/main/java/io/opentdf/platform/sdk/Autoconfigure.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java
| * operations and configurations. | ||
| */ | ||
| class TDF { | ||
| public class TDF { |
There was a problem hiding this comment.
TDF is public but still unusable outside the package.
External callers can now see TDF, but both constructors and all of the actual entry points (createTDF / loadTDF) remain package-private. As shipped, consumers can reference the type but cannot create or operate on one, so this public promotion is incomplete.
As per coding guidelines, keep public SDK APIs stable and additive where possible.
Also applies to: 71-80, 369-542
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/main/java/io/opentdf/platform/sdk/TDF.java` at line 34, TDF is
declared public but its constructors and entry-point factory methods are
package-private, making the type unusable to external callers; fix by making the
TDF constructors and all factory/entry methods (notably createTDF and loadTDF
and any overloads) public so external consumers can instantiate/use the class,
and ensure any other package-private methods in the TDF class that are intended
as part of the public SDK surface are likewise promoted to public (adjust
visibility for constructors and the methods referenced in the TDF class
accordingly).
| // Superclass and interfaces are part of the API surface of c. | ||
| c.getRawSuperclass().ifPresent(ec -> addAllRawTypes(c, work, queued)); | ||
| work.addAll(c.getRawInterfaces()); |
There was a problem hiding this comment.
Use the discovered superclass type here, not c.
Line 120 ignores ec and re-enqueues the current class, so exposed base classes never become reachable through subclass exposure. That can make this enforcement test misclassify public supertypes on both assertions.
Suggested fix
- c.getRawSuperclass().ifPresent(ec -> addAllRawTypes(c, work, queued));
+ c.getRawSuperclass().ifPresent(ec -> addAllRawTypes(ec, work, queued));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/test/java/io/opentdf/platform/sdk/PublicApiSurfaceTest.java` around
lines 119 - 121, The superclass lambda currently ignores the discovered
superclass value `ec` and re-enqueues `c`, so change the lambda to operate on
`ec`: call addAllRawTypes(ec, work, queued) (instead of addAllRawTypes(c,...))
and also use the superclass's interfaces by replacing the external
work.addAll(c.getRawInterfaces()) with work.addAll(ec.getRawInterfaces()) inside
the ifPresent block (or otherwise ensure you call getRawInterfaces() on `ec` not
`c`) so base classes become reachable through subclass exposure.
1 participant
We expose types only if they are reachable from things we want people to use
If a type is not transitively reachable from one of our root classes then we
do not expose it. We make an exception for exceptions.
All types that are exposed by things we want people to use are public
This allows clients to do explicit typing for names.
Summary by CodeRabbit
Chores
Bug Fixes
Tests