update transitive dependencies#369
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 by reducing the visibility of numerous classes and enums to package-private or internal, and updates InvalidZipException to extend SDKException. It also introduces ArchUnit for architectural testing. Feedback focuses on several critical accessibility issues in Manifest.java, where making inner classes package-private would cause compilation errors or break the public API due to their exposure in public fields and methods. A suggestion was also made to implement ArchUnit rules to automatically detect such API leaks in the future.
| } | ||
|
|
||
| static public class Segment { | ||
| static class Segment { |
There was a problem hiding this comment.
The Segment class is used in the segments field of IntegrityInformation (line 148). Since IntegrityInformation is a public static inner class and segments is a public field, making Segment package-private will cause accessibility issues or compilation errors for external consumers of the SDK.
| static class Segment { | |
| static public class Segment { |
| } | ||
|
|
||
| static public class Binding { | ||
| static class Binding { |
There was a problem hiding this comment.
The Binding class is used in the binding field of the Assertion class. If Assertion is restored to public visibility to resolve the API leak, Binding must also be public.
| static class Binding { | |
| static public class Binding { |
| } | ||
|
|
||
| static public class Assertion { | ||
| static class Assertion { |
There was a problem hiding this comment.
The Assertion class is used in the assertions field of the public Manifest class (line 560). In Java, a public field cannot expose a package-private type. This change will cause a compilation error and prevents external users from interacting with the assertions in a manifest.
| static class Assertion { | |
| static public class Assertion { |
| public AssertionConfig.Statement statement; | ||
| public Binding binding; | ||
| static public class HashValues { | ||
| static class HashValues { |
There was a problem hiding this comment.
The HashValues class is used as a return type for the verify method (line 406) and as a parameter for the sign method (line 379). Since these methods are public members of the Assertion class, HashValues must also be public to avoid compilation errors and ensure API usability.
| static class HashValues { | |
| static public class HashValues { |
| <groupId>com.tngtech.archunit</groupId> | ||
| <artifactId>archunit-junit5</artifactId> | ||
| <version>1.3.0</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| </dependencies> |
There was a problem hiding this comment.
The addition of ArchUnit is a great step for maintaining architectural integrity. Given the extensive visibility changes in this PR, you should consider adding ArchUnit rules to verify that internal implementation details (like package-private classes) do not leak through public API signatures or fields, which would help catch issues like the ones identified in Manifest.java.
| } | ||
|
|
||
| static public class PolicyBinding { | ||
| static class PolicyBinding { |
There was a problem hiding this comment.
For consistency with other components of the Manifest structure that are intended to be accessible, PolicyBinding should remain public. While it is currently exposed via an Object field in KeyAccess, consumers may need to cast it to access its properties.
| static class PolicyBinding { | |
| static public class PolicyBinding { |
X-Test Failure Report |
1 similar comment
X-Test Failure Report |
1 participant
No description provided.