Grpc Static Filter addition handling #123
Conversation
WalkthroughThis pull request updates the gRPC configuration and build settings. The Changes
Sequence Diagram(s)sequenceDiagram
participant GB as GuiceBundle
participant GS as GrpcServer
participant FI as FilterInterceptor
participant GC as GrpcConfig
GB->>GS: registerFilters(grpcFilters, bindableServices, configuration.getGrpc())
GS->>FI: registerFilters(grpcFilters, bindableServices, grpcConfig)
FI->>FI: addAllStaticFilters(grpcConfig, filtersForMethod, classToInstanceMap)
FI->>FI: Handle exceptions using trailers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (2)
82-86: Consider handling partial filter load failures more gracefully.
Currently, if a single filter class is missing, a RuntimeException is thrown and registration fails entirely. Depending on your use case, you may want to log the missing class and continue if feasible.
242-260: Validate dynamic loading of filter classes.
Using reflection to load classes can be expensive and may pose security risks if the input is not fully trusted. Consider verifying each loaded class actually implements GrpcFilter, and/or log a clear warning if it doesn’t.core/src/main/java/com/flipkart/gjex/core/config/GrpcConfig.java (1)
39-40: Filter classes property introduced.
Ensure that providing an empty or null list of filter classes won’t break filter registration logic. Also consider whether default values or validations are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (9)
build.gradle(1 hunks)core/src/main/java/com/flipkart/gjex/core/config/GrpcConfig.java(2 hunks)examples/src/main/resources/hello_world_config.yml(2 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)gradlew(4 hunks)gradlew.bat(4 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(4 hunks)guice/src/main/java/com/flipkart/gjex/grpc/service/GrpcServer.java(2 hunks)guice/src/main/java/com/flipkart/gjex/guice/GuiceBundle.java(1 hunks)
🔇 Additional comments (13)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (3)
18-18: New GrpcConfig import looks aligned with updated references.
No issues observed here.
74-74: Confirm usage changes for registerFilters signature.
Since the parameter switched from GrpcFilterConfig to GrpcConfig, verify all callers and references in the codebase are updated accordingly to avoid breakage.
204-205: Ensure trailers are not null when retrieving from StatusRuntimeException.
IfstatusRuntimeException.getTrailers()returns null, passing it tocall.closecould lead to unexpected behavior. Add a null check to handle this scenario gracefully.Also applies to: 207-209, 212-212
core/src/main/java/com/flipkart/gjex/core/config/GrpcConfig.java (1)
22-23: Import for List is proper and required.
No issues spotted.gradlew.bat (2)
17-17: Batch script condition syntax improvements.
Replacing “==” with “equ” for errorlevel checks is a standard best practice in Windows batch. The changes look correct.Also applies to: 28-28, 44-44, 79-79
84-87: Refined handling of exit codes.
Setting EXIT_CODE based on ERRORLEVEL and exiting with that code ensures consistent and accurate error reporting in the shell environment.guice/src/main/java/com/flipkart/gjex/grpc/service/GrpcServer.java (1)
117-118: LGTM! Method signature update aligns with broader configuration management.The change to use
GrpcConfiginstead ofGrpcFilterConfigprovides access to the complete gRPC configuration, enabling more flexible filter management.gradlew (2)
83-87: LGTM! Enhanced script robustness.Improved error handling by:
- Suppressing cd output when CDPATH is set
- Adding shellcheck directives for better maintainability
217-221: LGTM! Added xargs availability check.Added a critical check for xargs availability before proceeding with command parsing.
guice/src/main/java/com/flipkart/gjex/guice/GuiceBundle.java (1)
156-156: LGTM! Updated filter registration to use complete gRPC configuration.The change to pass
configuration.getGrpc()aligns with the updated method signature inGrpcServerand provides access to all gRPC settings.gradle/wrapper/gradle-wrapper.properties (1)
4-5: LGTM! Enhanced Gradle wrapper configuration.Added important network and security settings:
- Network timeout of 10 seconds prevents hanging on slow connections
- URL validation ensures secure distribution downloads
examples/src/main/resources/hello_world_config.yml (1)
4-5: New gRPC Filter Configuration Added
The addition of thefilterskey and corresponding list value (com.flipkart.gjex.core.filter.grpc.AccessLogGrpcFilter) is clear and follows the YAML structure expected for multi-value properties. Please verify that the YAML indentation conforms to your schema and that this new configuration properly maps to thefilterClassesproperty in your gRPC configuration class.build.gradle (1)
30-31: gJEX Version Update Validation
The update ofgJEXVersionto'1.45-SNAPSHOT'is an important version bump that aligns with the new filter handling changes. Ensure that all components relying on this version are compatible and that no backward compatibility issues arise with existing modules or builds.
Summary by CodeRabbit
New Features
Chores