static filter addition handling #124
Conversation
WalkthroughThe pull request updates the project’s versioning and enhances the gRPC filter configuration mechanism. It modifies the build configuration with a new version string, adds necessary imports, and introduces a new configuration entry for filter classes. The changes update the filter registration process in the interceptor with a simplified check for filter classes. Additionally, new configuration methods are added to several filter classes, and a Guice binding is introduced for dependency injection. A new field for filter classes is also added to the gRPC filter configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as GrpcFilterConfig
participant Interceptor as FilterInterceptor
participant Loader as addAllStaticFilters
participant Filter as GrpcFilter
Config->>Interceptor: Provide filter configuration (filterClasses list)
Interceptor->>Loader: Invoke addAllStaticFilters(config, filters list)
Loader->>Filter: Dynamically load filter class from config
Filter-->>Loader: Return filter instance (or error if class not found)
Loader-->>Interceptor: Add filter instance to filters list
Poem
✨ Finishing Touches
🪧 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 (2)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (2)
82-86: Add logging for better error tracking.Consider adding debug/info level logging when static filters are successfully added and error level logging when class loading fails.
try { addAllStaticFilters(grpcConfig, filtersForMethod, classToInstanceMap); + debug("Successfully added static filters for method"); } catch (ClassNotFoundException e) { + error("Failed to load filter class", e); throw new RuntimeException("Class not found :" + e.getMessage(), e); }
238-256: Optimize filter class loading and enhance error handling.The current implementation has several areas for improvement:
- Class loading is performed in a loop, which could be optimized
- Error handling could be more specific
- Null check on filterClasses could be moved to the method start
Consider applying these improvements:
private void addAllStaticFilters(GrpcConfig grpcConfig, List<GrpcFilter> filtersForMethod, Map<Class<?>, GrpcFilter> classToInstanceMap) throws ClassNotFoundException { List<String> filterClasses = grpcConfig.getFilterClasses(); - if (filterClasses != null && !filterClasses.isEmpty()) { - for (String filterClass : filterClasses) { - try { - Class<?> clazz = Class.forName(filterClass); - if (clazz == AccessLogGrpcFilter.class) { - configureAccessLog(grpcConfig.getGrpcFilterConfig(), filtersForMethod); - } else { - if (classToInstanceMap.containsKey(clazz)) { - filtersForMethod.add(classToInstanceMap.get(clazz)); - } - } - } catch (ClassNotFoundException e) { - throw new ClassNotFoundException(filterClass, e); - } + if (filterClasses == null || filterClasses.isEmpty()) { + return; + } + + for (String filterClass : filterClasses) { + Class<?> clazz; + try { + clazz = Class.forName(filterClass); + } catch (ClassNotFoundException e) { + error("Failed to load filter class: " + filterClass, e); + throw new ClassNotFoundException("Failed to load filter class: " + filterClass, e); } + + if (clazz == AccessLogGrpcFilter.class) { + configureAccessLog(grpcConfig.getGrpcFilterConfig(), filtersForMethod); + debug("Configured AccessLogGrpcFilter"); + continue; + } + + if (classToInstanceMap.containsKey(clazz)) { + filtersForMethod.add(classToInstanceMap.get(clazz)); + debug("Added filter: " + clazz.getSimpleName()); + } else { + warn("Filter class found but no instance available: " + clazz.getName()); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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(1 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(3 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 (6)
core/src/main/java/com/flipkart/gjex/core/config/GrpcConfig.java (1)
39-40: LGTM! New property added for filter configuration.The addition of
filterClassesproperty with@JsonProperty("filters")allows for flexible configuration of multiple gRPC filters.guice/src/main/java/com/flipkart/gjex/grpc/service/GrpcServer.java (1)
117-118: LGTM! Method signature updated to use broader configuration object.The change from
GrpcFilterConfigtoGrpcConfigprovides access to the complete gRPC configuration, which is necessary for the new static filter handling.guice/src/main/java/com/flipkart/gjex/guice/GuiceBundle.java (1)
156-156: LGTM! Updated to pass complete gRPC configuration.The change correctly passes the complete gRPC configuration object instead of just the filter config.
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (1)
238-256:✅ Verification successful
Verify filter class loading behavior.
Let's verify the behavior of filter class loading to ensure it works as expected.
🏁 Script executed:
#!/bin/bash # Description: Search for filter class implementations and their usage # Find all GrpcFilter implementations echo "=== GrpcFilter implementations ===" ast-grep --pattern 'class $_ implements GrpcFilter' # Find all filter class references in configuration files echo -e "\n=== Filter class references in configuration ===" rg -g '*.yml' -g '*.yaml' 'filters:'Length of output: 477
Filter Class Loading Behavior Verified
The investigation shows that the static filter-loading method in
FilterInterceptor.javais working as expected:
- Special Case Handling: The code correctly treats
AccessLogGrpcFilteras a special case by routing its configuration throughconfigureAccessLog().- Dynamic Loading: Other filter classes are only added if an instance is already available in the
classToInstanceMap, ensuring no duplicates or unintended instantiation.- Configuration References: The presence of filter-related entries in the YAML configuration (e.g., in
grpc-jexpress-template/envoy.ymlandexamples/src/main/resources/hello_world_config.yml) confirms that filter definitions are referenced properly.No issues were found with how filter classes are being loaded, so this review comment is resolved.
examples/src/main/resources/hello_world_config.yml (1)
4-5: New Filter Configuration Entry Added.
The addition of thefilterskey along with the list entrycom.flipkart.gjex.core.filter.grpc.AccessLogGrpcFilterclearly defines which static filter to load. This matches the revisedGrpcConfigdesign (mapped via@JsonProperty("filters")) and supports multiple filters. Please verify that the naming (“filters”) remains consistent with the rest of the system's configuration and documentation.build.gradle (1)
30-30: Updated Project Version Identifier.
The version string forgJEXVersionhas been updated to'1.43-SNAPSHOT-AUTH'. This update appears to align with the broader changes in the gRPC configuration. Ensure that theAUTHsuffix is intentional and documented as part of your versioning/release strategy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (2)
238-256: Add input validation for filter class names.While the method handles
filterClasseslist being null or empty, it should also validate individual class names to preventNullPointerExceptionor empty string issues.Consider this improvement:
private void addAllStaticFilters(GrpcConfig grpcConfig, List<GrpcFilter> filtersForMethod, Map<Class<?>, GrpcFilter> classToInstanceMap) throws ClassNotFoundException { List<String> filterClasses = grpcConfig.getFilterClasses(); if (filterClasses != null && !filterClasses.isEmpty()) { for (String filterClass : filterClasses) { + if (StringUtils.isBlank(filterClass)) { + continue; + } try { Class<?> clazz = Class.forName(filterClass);
227-236: Consider makingconfigureAccessLogprivate.The method appears to be package-private but is only used internally within the class. Consider making it private to better encapsulate the implementation.
-void configureAccessLog(GrpcFilterConfig grpcFilterConfig, +private void configureAccessLog(GrpcFilterConfig grpcFilterConfig,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(3 hunks)
🔇 Additional comments (2)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (2)
18-18: LGTM!The import is correctly added to support the new
GrpcConfigparameter type.
73-86:❓ Verification inconclusive
Verify error handling in caller code.
The method signature change from
GrpcFilterConfigtoGrpcConfigis good as it provides access to broader configuration options. However, the new error handling might impact caller code.Run this script to check if caller code handles the new
RuntimeException:
🏁 Script executed:
#!/bin/bash # Description: Find all callers of registerFilters to verify error handling # Search for method calls ast-grep --pattern 'filterInterceptor.registerFilters($$$)' # Search for try-catch blocks around the calls rg -A 5 'try\s*\{.*registerFilters.*\}'Length of output: 103
Attention: Verify caller error handling for updated exception propagation.
The method signature change from
GrpcFilterConfigtoGrpcConfigis a welcome improvement providing access to broader configuration options. However, the new error handling—wrapping aClassNotFoundExceptionin aRuntimeException—could impact caller code if not handled correctly. Since the automated search did not return results for callers, please manually verify that all invocations ofregisterFiltersare properly prepared to handle theRuntimeException.
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcFilter.java (1)
50-50: Add documentation for the configure method.The new abstract method lacks documentation explaining its purpose, parameters, and return value. This is especially important as it's part of the base filter class that other implementations will extend.
Add Javadoc for the method:
+ /** + * Configures the filter using the provided gRPC configuration + * @param grpcConfig The configuration object containing filter settings + * @return The configured filter instance for method chaining + */ public abstract GrpcFilter configure(GrpcConfig grpcConfig);examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/LoggingFilter.java (1)
42-45: Consider utilizing GrpcConfig for logging configuration.The current implementation returns
thiswithout using the provided configuration. Consider enhancing the filter to support configurable logging options such as log level, format, or filtering rules.Example implementation:
@Override public GrpcFilter configure(GrpcConfig grpcConfig) { + // Configure logging options from grpcConfig + if (grpcConfig.getLogLevel() != null) { + this.logLevel = grpcConfig.getLogLevel(); + } + if (grpcConfig.getLogFormat() != null) { + this.logFormat = grpcConfig.getLogFormat(); + } return this; }core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java (1)
149-160: Consider returningOptional<GrpcFilter>instead of null.The
configuremethod returnsnullwhen access logs are disabled. UsingOptionalwould make this behavior more explicit and help prevent potential null pointer exceptions.Apply this diff to improve null handling:
- public GrpcFilter configure(GrpcConfig grpcConfig) { + public Optional<GrpcFilter> configure(GrpcConfig grpcConfig) { GrpcFilterConfig grpcFilterConfig = grpcConfig.getGrpcFilterConfig(); if (grpcFilterConfig.isEnableAccessLogs()){ AccessLogGrpcFilter accessLogGrpcFilter = new AccessLogGrpcFilter(); if (org.apache.commons.lang.StringUtils.isNotBlank(grpcFilterConfig.getAccessLogFormat())){ AccessLogGrpcFilter.setFormat(grpcFilterConfig.getAccessLogFormat()); } - return accessLogGrpcFilter; + return Optional.of(accessLogGrpcFilter); } - return null; + return Optional.empty(); }guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (2)
82-95: Consider using Optional to handle null filters.The null check after
configurecould be improved using Optional to make the code more functional and explicit.Apply this diff to improve null handling:
try { addAllStaticFilters(grpcConfig, filtersForMethod, classToInstanceMap); } catch (ClassNotFoundException e) { throw new RuntimeException("Class not found :" + e.getMessage(), e); } Arrays.asList(pair.getValue().getAnnotation(MethodFilters.class).value()).forEach(filterClass -> { if (!classToInstanceMap.containsKey(filterClass)) { throw new RuntimeException("Filter instance not bound for Filter class :" + filterClass.getName()); } - GrpcFilter grpcFilter = classToInstanceMap.get(filterClass).configure(grpcConfig); - if (grpcFilter != null) { - filtersForMethod.add(grpcFilter); - } + classToInstanceMap.get(filterClass) + .configure(grpcConfig) + .ifPresent(filtersForMethod::add); });
230-247: Add input validation for method parameters.The method should validate its input parameters to prevent potential null pointer exceptions.
Apply this diff to add input validation:
private void addAllStaticFilters(GrpcConfig grpcConfig, List<GrpcFilter> filtersForMethod, Map<Class<?>, GrpcFilter> classToInstanceMap) throws ClassNotFoundException { + if (grpcConfig == null || filtersForMethod == null || classToInstanceMap == null) { + throw new IllegalArgumentException("Method parameters cannot be null"); + } List<String> filterClasses = grpcConfig.getFilterClasses(); if (filterClasses != null && !filterClasses.isEmpty()) { for (String filterClass : filterClasses) { try { Class<?> clazz = Class.forName(filterClass); if (classToInstanceMap.containsKey(clazz)) { - GrpcFilter filter = classToInstanceMap.get(clazz).configure(grpcConfig); - if (filter != null) { - filtersForMethod.add(filter); - } + classToInstanceMap.get(clazz) + .configure(grpcConfig) + .ifPresent(filtersForMethod::add); } } catch (ClassNotFoundException e) { throw new ClassNotFoundException(filterClass, e); } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.gradle(1 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java(4 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcFilter.java(2 hunks)examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/AuthFilter.java(2 hunks)examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/LoggingFilter.java(2 hunks)examples/src/main/java/com/flipkart/gjex/examples/helloworld/guice/HelloWorldModule.java(2 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build.gradle
🔇 Additional comments (3)
examples/src/main/java/com/flipkart/gjex/examples/helloworld/guice/HelloWorldModule.java (1)
57-57: LGTM!The binding for AccessLogGrpcFilter follows the same pattern as other filter bindings and is correctly implemented.
core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java (1)
18-18: LGTM!The added imports and
@Namedannotation support dependency injection and the new configuration functionality.Also applies to: 29-30, 48-48
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (1)
73-74: LGTM!The parameter change from
GrpcFilterConfigtoGrpcConfigaligns with the interface pattern and provides access to broader configuration options.
examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/AuthFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcFilterConfig.java (1)
40-42: Consider adding documentation for the new property.
The newfilterClassesfield broadens configurability and is a solid approach. However, adding a brief Javadoc explaining its purpose and usage can enhance readability and maintainability.guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (2)
88-91: Ensure thread safety when reconfiguring filters.
Callingconfigure(grpcFilterConfig)at registration time is fine, but check if any concurrency pitfalls occur when reusing filter instances across multiple services or methods.
227-244: Improve logging for unbound filters.
If a class is infilterClassesbut not inclassToInstanceMap, the filter is silently skipped. Consider logging a warning or throwing an exception to inform users that a filter was not recognized or never bound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/flipkart/gjex/core/config/GrpcConfig.java(1 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java(3 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcFilter.java(2 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcFilterConfig.java(2 hunks)examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/AuthFilter.java(2 hunks)examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/LoggingFilter.java(2 hunks)examples/src/main/resources/hello_world_config.yml(1 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/flipkart/gjex/core/config/GrpcConfig.java
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/LoggingFilter.java
- examples/src/main/resources/hello_world_config.yml
- examples/src/main/java/com/flipkart/gjex/examples/helloworld/filter/AuthFilter.java
- core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcFilter.java
🔇 Additional comments (3)
core/src/main/java/com/flipkart/gjex/core/filter/grpc/GrpcFilterConfig.java (1)
23-24: Good import usage for the new property.
Bringing inListensures clarity in the newly addedList<String>field. No issues here.core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java (1)
28-29: Annotations and imports are appropriate.
Applying@Named("AccessLogGrpcFilter")and importing the necessary classes aligns well with Guice or other DI frameworks.Also applies to: 47-47
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (1)
79-83: Confirm fail-fast behavior versus partial filter loading.
Currently, if any configured filter class fails to load, the entire registration process throws aRuntimeException. This ensures early detection at startup but completely blocks service availability if one filter is misconfigured. Keep or revise this approach depending on your operational needs.
core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java
Outdated
Show resolved
Hide resolved
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
guice/build.gradle (1)
64-65: New Dependency Addition: Apache Commons Collections 4.0
The new dependency on Apache Commons Collections version 4.0 is correctly added. Please verify that this version is compatible with the rest of the project, and consider checking for any known issues or vulnerabilities with this older release.guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (1)
236-239: Consolidate repeated logic and verify exception handling.These lines mirror the filter-configuration approach used earlier (lines 91–94). Consider extracting the logic that configures and adds filters into a helper method to avoid duplication. Additionally, ensure that
ClassNotFoundExceptionis either logged or rethrown for visibility and troubleshooting clarity rather than silently swallowed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.gradle(1 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(3 hunks)build.gradle(1 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(1 hunks)build.gradle(1 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java(4 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(2 hunks)build.gradle(1 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java(1 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(3 hunks)build.gradle(1 hunks)build.gradle(1 hunks)core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java(1 hunks)guice/build.gradle(1 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java
- build.gradle
- build.gradle
- build.gradle
- build.gradle
- guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java
- core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java
- build.gradle
- core/src/main/java/com/flipkart/gjex/core/filter/grpc/AccessLogGrpcFilter.java
- build.gradle
- guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (2)
guice/build.gradle (1)
1-87: Verify Version Update Consistency
The PR summary mentions an update to thegJEXVersionvariable (from'1.43-SNAPSHOT-3'to'1.43-SNAPSHOT-4'), yet this change isn’t visible in the provided diff of this file. Please confirm whether the version update was intended for this file or if it is applied elsewhere.guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (1)
91-94:❓ Verification inconclusive
Potential mismatch in the parameter reference.
Here,
classToInstanceMap.get(filterClass).configure(grpcConfig)is usinggrpcConfig, while theregisterFiltersmethod signature only declaresgrpcFilterConfig. IfGrpcFilterConfigdiffers fromGrpcConfig(or doesn’t inherit it), this could lead to a compilation error or unintended behavior. Please confirm whether the intended parameter here is actuallygrpcFilterConfigor if the method signature should be updated to acceptGrpcConfig.As a quick fix, if your intention was to use the same parameter declared in the method signature, apply this diff:
- GrpcFilter grpcFilter = classToInstanceMap.get(filterClass).configure(grpcConfig); + GrpcFilter grpcFilter = classToInstanceMap.get(filterClass).configure(grpcFilterConfig);
Below are shell script commands to verify the parameter usage in the
FilterInterceptor.javafile. These commands will search for theregisterFiltersmethod declaration and the usage of bothgrpcConfigandgrpcFilterConfigwithin the file, so we can confirm whether the mismatch noted in the review comment actually exists.Please run the following shell scripts:
#!/bin/bash echo "=== Extracting the registerFilters method context ===" rg "registerFilters" -A 20 guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java echo "=== Searching for occurrences of 'grpcConfig' ===" rg "grpcConfig" -n guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java echo "=== Searching for occurrences of 'grpcFilterConfig' ===" rg "grpcFilterConfig" -n guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.javaOnce you have the outputs from these queries, please verify whether:
- The
registerFiltersmethod’s signature indeed declares a parameter namedgrpcFilterConfig(and notgrpcConfig).- The
configure(...)method inside the lambda is being called withgrpcConfigrather than the declaredgrpcFilterConfig.This verification will determine if applying the diff
- GrpcFilter grpcFilter = classToInstanceMap.get(filterClass).configure(grpcConfig); + GrpcFilter grpcFilter = classToInstanceMap.get(filterClass).configure(grpcFilterConfig);is required.
Please share the results, or confirm if the mismatch is validated.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (2)
180-184: Remove commented-out code.The commented-out line 184 should be removed as it's no longer needed. Having commented code can lead to confusion and maintenance issues.
- for (GrpcFilter filter : grpcFilters) { - filter.doProcessRequest(request, requestParams); - } - -// grpcFilters.forEach(filter -> filter.doProcessRequest(request, requestParams)); + for (GrpcFilter filter : grpcFilters) { + filter.doProcessRequest(request, requestParams); + }
247-264: Well-implemented static filter configuration mechanism.The new
addAllStaticFiltersmethod effectively:
- Retrieves filter classes from configuration
- Uses CollectionUtils for readable empty checks
- Handles class loading safely
- Configures and adds filters when found
One suggestion: consider using a more descriptive variable name than
clazzto improve readability.- Class<?> clazz = Class.forName(filterClass); - if (classToInstanceMap.containsKey(clazz)) { - GrpcFilter filter = classToInstanceMap.get(clazz).configure(grpcFilterConfig); + Class<?> filterClassType = Class.forName(filterClass); + if (classToInstanceMap.containsKey(filterClassType)) { + GrpcFilter filter = classToInstanceMap.get(filterClassType).configure(grpcFilterConfig);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.gradle(1 hunks)core/src/main/java/com/flipkart/gjex/core/filter/Filter.java(2 hunks)guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build.gradle
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: publish
🔇 Additional comments (7)
guice/src/main/java/com/flipkart/gjex/grpc/interceptor/FilterInterceptor.java (6)
37-40: New imports for enhanced exception handling and collection utilities.The additions of
StatusExceptionandCollectionUtilsare appropriate for the new functionality being implemented.
82-86: Good error handling for static filter configuration.The try-catch block properly handles class loading failures with detailed error messages.
91-94: Improved filter configuration with null safety check.The code now properly calls
configure()on each filter and safely handles potential null returns.
186-189: Good enhancement for StatusException handling.Adding specific handling for StatusException improves error management throughout the filter chain.
215-225: Comprehensive exception handling with proper trailer extraction.The enhanced exception handling now properly distinguishes between different exception types and correctly extracts trailers from both StatusRuntimeException and StatusException.
228-228: Improved error response with metadata preservation.The call.close method now properly includes metadata from the original exception.
core/src/main/java/com/flipkart/gjex/core/filter/Filter.java (1)
18-19: Appropriate import addition for enhanced error handling.The addition of the
io.grpc.StatusExceptionimport is necessary to support the modified method signature that enables proper gRPC error handling.
| @@ -40,7 +42,7 @@ public void destroy() {} | |||
| * @param req the Request body/message | |||
| * @param requestParams prams | |||
| */ | |||
| public void doProcessRequest(Req req, RequestParams<M> requestParams) {} | |||
| public void doProcessRequest(Req req, RequestParams<M> requestParams) throws StatusException {} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update JavaDoc to reflect the new exception.
The method signature has been correctly updated to throw StatusException, which allows implementations to signal gRPC-specific failures. However, the JavaDoc comment above (lines 38-40) contradicts this change by stating "This Filter cannot fail processing of the Request body and hence there is no support for indicating failure."
/**
* Call-back to decorate or inspect the Request body/message. This Filter cannot fail processing of the Request body and hence there is no support for indicating failure.
* This method should be viewed almost like a proxy for the Request body.
*
* @param req the Request body/message
* @param requestParams prams
+ * @throws StatusException if the filter rejects the request based on gRPC status codes
*/
public void doProcessRequest(Req req, RequestParams<M> requestParams) throws StatusException {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void doProcessRequest(Req req, RequestParams<M> requestParams) throws StatusException {} | |
| /** | |
| * Call-back to decorate or inspect the Request body/message. This Filter cannot fail processing of the Request body and hence there is no support for indicating failure. | |
| * This method should be viewed almost like a proxy for the Request body. | |
| * | |
| * @param req the Request body/message | |
| * @param requestParams prams | |
| * @throws StatusException if the filter rejects the request based on gRPC status codes | |
| */ | |
| public void doProcessRequest(Req req, RequestParams<M> requestParams) throws StatusException {} |
💡 Verification agent
🧩 Analysis chain
Breaking change - consider migration strategy.
Adding the throws StatusException declaration is a breaking change that will require all implementations of this method to handle or propagate this exception. Ensure that:
- All existing implementations are updated
- A migration guide is provided
- Consider if this warrants a major version increment
🏁 Script executed:
#!/bin/bash
# Find all implementations of the doProcessRequest method
rg -A 1 -B 1 "doProcessRequest\s*\(\s*\w+\s+\w+\s*,\s*RequestParams\s*<\s*\w+\s*>\s+\w+\s*\)\s*\{" --type javaLength of output: 1785
Breaking change in Filter#doProcessRequest – migration strategy required
The addition of throws StatusException to the signature of Filter#doProcessRequest now forces all callers of this API to handle or propagate this checked exception. Note that while overridden implementations (e.g., in GetLoggingFilter.java, CreateLoggingFilter.java, LoggingFilter.java, and AccessLogGrpcFilter.java) do not explicitly include the throws StatusException clause, this is acceptable under Java’s overriding rules if they don’t actually throw the exception. Nonetheless, please ensure the following:
- Review and Update Client Code: Confirm that all consumers of the
doProcessRequestAPI are updated to handle or propagateStatusException. - Provide a Migration Guide: Document the breaking change and outline steps to migrate existing usage.
- Versioning Consideration: Evaluate whether this API change warrants a major version bump.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
guice/src/main/java/com/flipkart/gjex/http/interceptor/HttpFilterInterceptor.java (2)
4-4: Remove unused import.The import
com.flipkart.gjex.core.filter.grpc.GrpcFilterappears to be unused in this file. Consider removing it to maintain code cleanliness.-import com.flipkart.gjex.core.filter.grpc.GrpcFilter;
88-91: Remove commented out code.The old implementation using
forEachis commented out but should be removed completely instead of being left as a comment. This improves code readability and prevents confusion.for (HttpFilter filter: filters) { filter.doProcessRequest(request, requestParams); } -// filters.forEach(filter -> filter.doProcessRequest(request, requestParams));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
guice/src/main/java/com/flipkart/gjex/http/interceptor/HttpFilterInterceptor.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
guice/src/main/java/com/flipkart/gjex/http/interceptor/HttpFilterInterceptor.java (1)
Learnt from: kingster
PR: flipkart-incubator/grpc-jexpress#120
File: guice/src/main/java/com/flipkart/gjex/http/interceptor/HttpFilterInterceptor.java:96-101
Timestamp: 2025-01-16T12:05:05.165Z
Learning: In the HttpFilterInterceptor class of grpc-jexpress, exception handling for HTTP responses is already implemented at a higher level outside the doFilter method, making additional error recovery mechanisms within doFilter unnecessary. The logging of errors within doFilter is sufficient.
🔇 Additional comments (2)
guice/src/main/java/com/flipkart/gjex/http/interceptor/HttpFilterInterceptor.java (2)
87-91: Code change looks good.The change from using a lambda with
forEachto a traditionalforloop is a reasonable modification that maintains the same functionality while potentially improving readability and debugging.
99-100: Verify exception handling approach.Based on previous feedback, exception handling for HTTP responses is already implemented at a higher level outside the
doFiltermethod. Please verify that this additionalStatusExceptionhandling aligns with the project's error handling strategy.This new catch block allows filters to handle exceptions specifically, which might be the intended behavior, but should be consistent with the overall architecture.
Summary by CodeRabbit
Chores
New Features