#214 Feature filter support added #243
Conversation
WalkthroughThis pull request primarily consists of non-functional improvements. It reorganizes and refactors import statements and formatting elements across several files, adds a new test runtime dependency to the build file, and adjusts testing methods. Additionally, it introduces a new filtering mechanism with a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FilterSerializer
participant ConditionEvaluator
Client->>FilterSerializer: Send JSON filter condition
FilterSerializer->>FilterSerializer: Deserialize JSON into Condition object
FilterSerializer->>ConditionEvaluator: Forward headers for evaluation
ConditionEvaluator-->>FilterSerializer: Return evaluation result (true/false)
FilterSerializer-->>Client: Deliver evaluation result
Suggested reviewers
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: 9
🔭 Outside diff range comments (2)
pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarMessageTest.java (1)
53-53:⚠️ Potential issueFix incorrect assertion.
The assertion is comparing a value with itself, which renders the test ineffective for this particular check.
- Assertions.assertEquals(producerMessage.getGroupId(), producerMessage.getGroupId()); + Assertions.assertEquals(producerMessage.getGroupId(), pulsarMessage.getGroupId());server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java (1)
51-63: 💡 Verification agent🧩 Analysis chain
Verify actual header retrieval in production code.
The test is setting headers and verifying the message ID is returned correctly, but it doesn't verify that the system actually reads and processes these headers properly. Consider adding assertions that verify the headers are correctly processed by the system.
🏁 Script executed:
#!/bin/bash # Description: Verify if the system uses HeaderUtils to process headers in production code echo "Checking references to MessageHeaders.MSG_ID in producer and handler code..." rg "MessageHeaders\.MSG_ID" --type java -g "**/*Producer.java" -g "**/*Handler.java" echo "Checking usage of HeaderUtils.getHeader in code that processes requests..." rg "HeaderUtils\.getHeader" --type java -g "**/*Handler.java" -g "**/*Controller.java"Length of output: 1358
Action Required: Expand Test Assertions for Header Processing
While production code correctly uses
HeaderUtils.getHeader(as seen inPreProduceHandler.java) to retrieve headers, the current tests inBodyHandlerTest.java(lines 51–63) only check that the message ID remains unchanged. They do not confirm that the headers are actually processed by the system.
- Please add explicit assertions that validate the header extraction and processing logic.
- Consider verifying not only the persistence of the message ID but also that both
MessageHeaders.MSG_IDand any other related headers (likeGROUP_ID) are correctly handled throughout the request lifecycle.
🧹 Nitpick comments (37)
pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarMessageTest.java (1)
27-61: Consider adding test cleanup for global state.The test initializes
HeaderUtilswhich appears to maintain global state. Consider adding cleanup after the test to prevent potential side effects on other tests.@Test void testPulsarMessagesEqualsProducerMessage() { // test request headers Multimap<String, String> requestHeaders = ArrayListMultimap.create(); MessageHeaderConfiguration messageHeaderConfiguration = MessageHeaderUtils.fetchDummyHeaderConfiguration(); HeaderUtils.initialize(messageHeaderConfiguration); // ... test code ... + + // After the test, reset the HeaderUtils to prevent side effects on other tests + try { + // Reset HeaderUtils state if a reset method is available + // HeaderUtils.reset(); + } catch (Exception e) { + // Log or handle any cleanup errors + } }If there's no public reset method available, consider suggesting the addition of one to improve test isolation.
consumer/src/main/java/com/flipkart/varadhi/consumer/processing/ProcessingLoop.java (2)
174-174: Inconsistent message ID retrieval approach.While line 146 was updated to use
HeaderUtils.getHeader(MessageHeaders.MSG_ID), this line still uses the directgetMessageId()method. For consistency with the new header management approach, consider updating this to use the same HeaderUtils approach.- message.getMessage().getMessageId(), + message.getMessage().getHeader(HeaderUtils.getHeader(MessageHeaders.MSG_ID)),
146-147: Code readability could be improved.The nested call structure makes the code slightly harder to read. Consider extracting the header key to a variable for better readability.
- msg.getMessage().getHeader(HeaderUtils.getHeader(MessageHeaders.MSG_ID)), + String msgIdHeader = HeaderUtils.getHeader(MessageHeaders.MSG_ID); + msg.getMessage().getHeader(msgIdHeader),entities/src/main/java/com/flipkart/varadhi/entities/DlqMessage.java (2)
29-29: Consider clarifying the method chain for better readability.The nested calls to
getHeadermethods could be confusing to readers.HeaderUtils.getHeader(MessageHeaders.MSG_ID)presumably returns a string key, which is then passed to the instance methodgetHeader(String key). This double use of similarly named methods reduces code clarity.Consider using a more descriptive variable or splitting this into multiple steps:
- return getHeader(HeaderUtils.getHeader(MessageHeaders.MSG_ID)); + String messageIdKey = HeaderUtils.getHeader(MessageHeaders.MSG_ID); + return getHeader(messageIdKey);
35-35: Consider clarifying the method chain for better readability.Similar to the
getMessageId()method, this nested call to similarly named methods could be confusing.Consider using a more descriptive variable or splitting this into multiple steps:
- return getHeader(HeaderUtils.getHeader(MessageHeaders.GROUP_ID)); + String groupIdKey = HeaderUtils.getHeader(MessageHeaders.GROUP_ID); + return getHeader(groupIdKey);entities/src/main/java/com/flipkart/varadhi/entities/constants/MessageHeaders.java (2)
1-4: Consider adding Javadoc to document the purpose of this enum.Adding class-level Javadoc would help developers understand the purpose and usage of this enum, especially since it replaces multiple legacy header classes.
package com.flipkart.varadhi.entities.constants; import com.fasterxml.jackson.annotation.JsonProperty; +/** + * Defines the standard message headers used throughout the application. + * This enum replaces the legacy header constants previously defined in + * separate classes like StandardHeaders and MessageConstants. + */ public enum MessageHeaders {
7-31: Add header descriptions for better documentation.Consider adding Javadoc comments to each enum constant to document its purpose, expected format, and any validation rules.
- @JsonProperty ("msgIdHeader") MSG_ID, + /** + * Unique identifier for a message. + */ + @JsonProperty ("msgIdHeader") MSG_ID, - @JsonProperty ("groupIdHeader") GROUP_ID, + /** + * Identifier for grouping related messages. + */ + @JsonProperty ("groupIdHeader") GROUP_ID,entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/EndsWithCondition.java (2)
8-15: Add parameter validation for null or empty values.The constructor doesn't validate if the parameters are null or empty strings. Consider adding validation to ensure robust behavior.
public record EndsWithCondition(String key, String value) implements Condition { @JsonCreator public EndsWithCondition(@JsonProperty (value = "key", required = true) String key, @JsonProperty (value = "value", required = true) String value) { + if (key == null || key.isEmpty()) { + throw new IllegalArgumentException("Key cannot be null or empty"); + } + if (value == null) { + throw new IllegalArgumentException("Value cannot be null"); + } this.key = key; this.value = value; }
1-7: Consider adding case-insensitive option for header matching.Header values might require case-insensitive comparison in some scenarios. A flag for case-insensitive matching would make this class more versatile.
package com.flipkart.varadhi.entities.filters.conditions.logical; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.flipkart.varadhi.entities.filters.conditions.Condition; import com.google.common.collect.Multimap; -public record EndsWithCondition(String key, String value) implements Condition { +public record EndsWithCondition(String key, String value, boolean ignoreCase) implements Condition { @JsonCreator public EndsWithCondition(@JsonProperty (value = "key", required = true) String key, @JsonProperty (value = "value", required = true) - String value) { + String value, @JsonProperty(value = "ignoreCase", required = false) + Boolean ignoreCase) { this.key = key; this.value = value; + this.ignoreCase = ignoreCase != null ? ignoreCase : false; }Then update the evaluate method to use the ignoreCase flag:
@Override public boolean evaluate(Multimap<String, String> headers) { - return headers.containsKey(key) && headers.get(key).stream().anyMatch(headerValue -> headerValue.endsWith(value)); + return headers.containsKey(key) && headers.get(key).stream().anyMatch(headerValue -> + ignoreCase ? headerValue.toLowerCase().endsWith(value.toLowerCase()) : headerValue.endsWith(value)); }entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ExistsCondition.java (2)
8-13: Add parameter validation for null or empty key.The constructor doesn't validate if the key parameter is null or an empty string. Consider adding validation to ensure robust behavior.
public record ExistsCondition(String key) implements Condition { @JsonCreator public ExistsCondition(@JsonProperty (value = "key", required = true) String key) { + if (key == null || key.isEmpty()) { + throw new IllegalArgumentException("Key cannot be null or empty"); + } this.key = key; }
1-7: Consider adding documentation for the class.Adding JavaDoc to describe the purpose and usage of this condition would improve code maintainability and help other developers understand how to use it properly.
package com.flipkart.varadhi.entities.filters.conditions.logical; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.flipkart.varadhi.entities.filters.conditions.Condition; import com.google.common.collect.Multimap; +/** + * A condition that evaluates to true if a specified header key exists in the message headers. + * This condition doesn't check the header values, only the existence of the key. + */ public record ExistsCondition(String key) implements Condition {entities/src/main/java/com/flipkart/varadhi/entities/utils/HeaderUtils.java (2)
12-19: Consider encapsulating public static fieldsThe utility class exposes public static fields (
headerValueSizeMaxandmaxRequestSize) which violates encapsulation principles. Consider making these private with getter methods to maintain better control over the internal state.- public static Integer headerValueSizeMax; - public static Integer maxRequestSize; + private static Integer headerValueSizeMax; + private static Integer maxRequestSize; + + public static Integer getHeaderValueSizeMax() { + checkInitialization(); + return headerValueSizeMax; + } + + public static Integer getMaxRequestSize() { + checkInitialization(); + return maxRequestSize; + }
50-60: Consider enhancing the required headers methodCurrently, the method hardcodes a single required header (MSG_ID). If more required headers may be added in the future, consider making this configurable via the MessageHeaderConfiguration.
public static List<String> getRequiredHeaders() { - return List.of(HeaderUtils.getHeader(MessageHeaders.MSG_ID)); + checkInitialization(); + // Example approach to make required headers configurable + return mapping.entrySet().stream() + .filter(entry -> entry.getKey().isRequired()) + .map(Map.Entry::getValue) + .collect(Collectors.toList()); }Note: This assumes MessageHeaders enum has or could have an
isRequired()method.server/src/main/java/com/flipkart/varadhi/web/RequestTelemetryConfigurator.java (2)
116-117: Optimize header retrieval to avoid duplicate calls.The current implementation calls
HeaderUtils.getHeader(MessageHeaders.MSG_ID)twice - once to check if the header exists and once to retrieve its value. This duplication is inefficient.- if (null != ctx.request().getHeader(HeaderUtils.getHeader(MessageHeaders.MSG_ID))) { - tags.add(Tag.of("message.id", ctx.request().getHeader(HeaderUtils.getHeader(MessageHeaders.MSG_ID)))); + String msgIdHeader = HeaderUtils.getHeader(MessageHeaders.MSG_ID); + String msgId = ctx.request().getHeader(msgIdHeader); + if (null != msgId) { + tags.add(Tag.of("message.id", msgId));
119-120: Optimize header retrieval to avoid duplicate calls.Similar to the message ID handling, the group ID header retrieval also makes duplicate calls to
HeaderUtils.getHeader().- if (null != ctx.request().getHeader(HeaderUtils.getHeader(MessageHeaders.GROUP_ID))) { - tags.add(Tag.of("group.id", ctx.request().getHeader(HeaderUtils.getHeader(MessageHeaders.GROUP_ID)))); + String groupIdHeader = HeaderUtils.getHeader(MessageHeaders.GROUP_ID); + String groupId = ctx.request().getHeader(groupIdHeader); + if (null != groupId) { + tags.add(Tag.of("group.id", groupId));entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NotCondition.java (3)
11-15: Rename field for semantic clarity.The field name
valuessuggests it contains multiple conditions, but it actually holds a singleCondition. Consider renaming it tovalueorconditionto better reflect its purpose.@Getter @Setter @SuperBuilder public class NotCondition implements Condition { - private Condition values; + private Condition condition; }
17-21: Update parameter name to match field name.The constructor parameter name (
value) doesn't match the field name (values), which could cause confusion. Ensure consistency in naming.@JsonCreator public NotCondition(@JsonProperty (value = "values", required = true) -Condition value) { - this.values = value; +Condition values) { + this.values = values; }Note: If you implement the previous suggestion to rename the field, you should also update the
@JsonPropertyvalue and adjust this constructor accordingly.
23-31: Update field references in methods.If you rename the
valuesfield, make sure to update the references in bothtoString()andevaluate()methods.@Override public String toString() { - return "not(" + values.toString() + ")"; + return "not(" + condition.toString() + ")"; } @Override public boolean evaluate(Multimap<String, String> headers) { - return !values.evaluate(headers); + return !condition.evaluate(headers); }entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/OrCondition.java (1)
1-2: Package inconsistency with class semantics.This class is placed in the
comparisonpackage, but it represents a logical operator rather than a comparison operation. Consider moving it to thelogicalpackage alongside other logical operators.-package com.flipkart.varadhi.entities.filters.conditions.comparison; +package com.flipkart.varadhi.entities.filters.conditions.logical;entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/AndCondition.java (2)
14-18: Consider validating the list of conditionsThe class is well-structured with appropriate Lombok annotations. However, there's no validation to ensure that the
valueslist is not empty. An empty list would always evaluate to true forallMatch, which might not be the intended behavior for an AND condition.@Getter @Setter @SuperBuilder public class AndCondition implements Condition { - private List<Condition> values; + private List<Condition> values; + + private void validateValues(List<Condition> values) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("AND condition requires at least one condition"); + } + }
20-24: Add validation in the constructorThe constructor correctly uses JsonCreator and JsonProperty annotations, but lacks validation for the input values.
@JsonCreator public AndCondition(@JsonProperty (value = "values", required = true) List<Condition> values) { + validateValues(values); this.values = values; }entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NandCondition.java (2)
14-18: Consider validating the list of conditionsThe class is well-structured with appropriate Lombok annotations. However, there's no validation to ensure that the
valueslist is not empty. An empty list would always evaluate to false for negatedallMatch, which might lead to unexpected behavior.@Getter @Setter @SuperBuilder public class NandCondition implements Condition { - private List<Condition> values; + private List<Condition> values; + + private void validateValues(List<Condition> values) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("NAND condition requires at least one condition"); + } + }
20-24: Add validation in the constructorThe constructor correctly uses JsonCreator and JsonProperty annotations, but lacks validation for the input values.
@JsonCreator public NandCondition(@JsonProperty (value = "values", required = true) List<Condition> values) { + validateValues(values); this.values = values; }server/src/main/java/com/flipkart/varadhi/config/AppConfiguration.java (1)
72-73: Consider adding error handling for initializationThe initialization and checking of header utils lack explicit error handling. Consider adding try-catch blocks or documentation about the expected behavior when initialization fails.
@Override public void validate() { Validatable.super.validate(); - HeaderUtils.initialize(messageHeaderConfiguration); - HeaderUtils.checkInitialization(); + try { + HeaderUtils.initialize(messageHeaderConfiguration); + HeaderUtils.checkInitialization(); + } catch (Exception e) { + throw new IllegalStateException("Failed to initialize header configuration: " + e.getMessage(), e); + } }entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NorCondition.java (2)
14-18: Consider validating the list of conditionsThe class is well-structured with appropriate Lombok annotations. However, there's no validation to ensure that the
valueslist is not empty. An empty list would always evaluate to true fornoneMatch, which might not be the intended behavior for a NOR condition.@Getter @Setter @SuperBuilder public class NorCondition implements Condition { - private List<Condition> values; + private List<Condition> values; + + private void validateValues(List<Condition> values) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("NOR condition requires at least one condition"); + } + }
20-24: Add validation in the constructorThe constructor correctly uses JsonCreator and JsonProperty annotations, but lacks validation for the input values.
@JsonCreator public NorCondition(@JsonProperty (value = "values", required = true) List<Condition> values) { + validateValues(values); this.values = values; }conf/configuration.yml (1)
86-103: Header configuration structure looks good, but consider refining some parameter names.The new nested
mappingstructure with header configurations and additional parameters is well-organized. The booleanfilterNonCompliantHeadersand size limits provide good control over header validation.Consider renaming some parameters for better clarity:
headerValueSizeMax→maxHeaderValueSize(more consistent withmaxRequestSize)filterNonCompliantHeaders→removeNonCompliantHeaders(more explicit about the action taken)entities/src/main/java/com/flipkart/varadhi/entities/config/MessageHeaderConfiguration.java (1)
52-54: Consider handling null in the prefix check method.The
startsWithValidPrefixmethod might encounter aNullPointerExceptionif the value parameter is null, although the prior validation should prevent this.private boolean startsWithValidPrefix(String value) { + if (value == null) { + return false; + } return allowedPrefix.stream().anyMatch(value::startsWith); }entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java (2)
19-19: Class name has a typo.There's a typo in the class name "FilterDeserialzeAndEvaluatorTest" (should be "Deserialize").
-public class FilterDeserialzeAndEvaluatorTest { +public class FilterDeserializeAndEvaluatorTest {
90-97: Malformed condition test cases should be more comprehensive.The test cases for malformed conditions are good, but could be expanded to cover more error scenarios.
Consider adding more test cases for malformed conditions such as:
- Invalid operator names for non-logical operations
- Empty values array for logical operations
- Missing key for operations that require it
- Invalid value types (numeric instead of string)
producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java (1)
311-311: Consider using a constant for the "ANONYMOUS" string.The hardcoded "ANONYMOUS" string is used for metrics tags. Consider defining a constant to avoid repeating this string literal.
Extract "ANONYMOUS" to a private static final constant at the class level:
public class ProducerServiceTests { + private static final String ANONYMOUS_IDENTITY = "ANONYMOUS"; ProducerService service; // ... // Then use it in this line: - produceAttributes.put(TAG_IDENTITY, "ANONYMOUS"); + produceAttributes.put(TAG_IDENTITY, ANONYMOUS_IDENTITY); // And in getMessage method: - headers.put(HeaderUtils.getHeader(MessageHeaders.PRODUCE_IDENTITY), "ANONYMOUS"); + headers.put(HeaderUtils.getHeader(MessageHeaders.PRODUCE_IDENTITY), ANONYMOUS_IDENTITY); }entities/src/test/java/com/flipkart/varadhi/utils/HeaderUtilsTest.java (2)
25-44: Enhance test assertion for required headers validation.The test verifies that an exception is thrown when required headers are missing, but doesn't assert which specific headers are missing or validate the exception message content.
IllegalArgumentException ae = Assertions.assertThrows( IllegalArgumentException.class, () -> HeaderUtils.ensureRequiredHeaders(varadhiHeaders) ); +// Assert that the exception message contains information about the missing headers +Assertions.assertTrue(ae.getMessage().contains(HeaderUtils.getHeader(MessageHeaders.MSG_ID)) || + ae.getMessage().contains(HeaderUtils.getHeader(MessageHeaders.PRODUCE_REGION)), + "Exception should mention the specific missing required headers");
80-81: Clarify intention of duplicate value test.Line 81 appears to be testing for the presence of a duplicate value after removal, but the code and intent aren't clear. If this is intentional, a comment would help explain the purpose.
multi_value2.remove("multi_Value2_1"); +// Asserting duplicate value still exists after removing one instance Assertions.assertTrue(multi_value2.contains("multi_Value2_1"));server/src/main/java/com/flipkart/varadhi/web/v1/produce/PreProduceHandler.java (1)
25-32: Consider using requestHeaders for size calculation to avoid redundant iteration.The method converts
headerstorequestHeadersfor validation, but then iterates through the originalheadersagain for size calculation. Using the same collection for both operations would be more efficient.private void ensureHeaderSemanticsAndSize(MultiMap headers, long bodyLength) { Multimap<String, String> requestHeaders = ArrayListMultimap.create(); headers.entries().forEach(entry -> { String key = entry.getKey(); requestHeaders.put(key, entry.getValue()); }); HeaderUtils.ensureRequiredHeaders(requestHeaders); long headersAndBodySize = 0; + + // Use requestHeaders for size calculation instead of iterating headers againserver/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (1)
87-91: Add descriptive assertions messages for header case sensitivity tests.These assertions test important behavior regarding header case sensitivity, but lack descriptive messages that explain what's being verified.
- Assertions.assertFalse(capturedMessage.hasHeader("RandomHeader".toUpperCase())); - Assertions.assertTrue(capturedMessage.getHeaders("x_header1".toUpperCase()).contains("h1v1")); - Assertions.assertTrue(capturedMessage.getHeaders("x_header1".toUpperCase()).contains("h1v2")); - Assertions.assertTrue(capturedMessage.getHeaders("x_header2".toUpperCase()).contains("h2v1")); - Assertions.assertTrue(capturedMessage.hasHeader("X_HEADER2")); + Assertions.assertFalse(capturedMessage.hasHeader("RandomHeader".toUpperCase()), + "Non-compliant headers should be filtered out"); + Assertions.assertTrue(capturedMessage.getHeaders("x_header1".toUpperCase()).contains("h1v1"), + "Headers should be retrievable case-insensitively"); + Assertions.assertTrue(capturedMessage.getHeaders("x_header1".toUpperCase()).contains("h1v2"), + "All values for a multi-value header should be preserved"); + Assertions.assertTrue(capturedMessage.getHeaders("x_header2".toUpperCase()).contains("h2v1"), + "Headers should be retrievable regardless of original case"); + Assertions.assertTrue(capturedMessage.hasHeader("X_HEADER2"), + "Headers should be accessible with their uppercase key");entities/src/testFixtures/java/com/flipkart/varadhi/entities/MessageHeaderUtils.java (2)
11-33: Add documentation for configuration parameters.The
fetchConfigurationmethod contains important parameters that would benefit from documentation explaining their purpose, especially the numeric values like 100 and 510241024.public static MessageHeaderConfiguration fetchConfiguration(boolean filterNonCompliantHeaders) { + // Create a configuration with: + // - A map of MessageHeaders enum values to their corresponding header names + // - List of valid header prefixes + // - Maximum allowed header value size (100 characters) + // - Maximum request size (5MB) + // - Flag to filter out non-compliant headers return new MessageHeaderConfiguration( Map.ofEntries( Map.entry(MessageHeaders.MSG_ID, "X_MESSAGE_ID"), Map.entry(MessageHeaders.GROUP_ID, "X_GROUP_ID"), Map.entry(MessageHeaders.CALLBACK_CODE, "X_CALLBACK_CODES"), Map.entry(MessageHeaders.REQUEST_TIMEOUT, "X_REQUEST_TIMEOUT"), Map.entry(MessageHeaders.REPLY_TO_HTTP_URI, "X_REPLY_TO_HTTP_URI"), Map.entry(MessageHeaders.REPLY_TO_HTTP_METHOD, "X_REPLY_TO_HTTP_METHOD"), Map.entry(MessageHeaders.REPLY_TO, "X_REPLY_TO"), Map.entry(MessageHeaders.HTTP_URI, "X_HTTP_URI"), Map.entry(MessageHeaders.HTTP_METHOD, "X_HTTP_METHOD"), Map.entry(MessageHeaders.CONTENT_TYPE, "X_CONTENT_TYPE"), Map.entry(MessageHeaders.PRODUCE_IDENTITY, "X_PRODUCE_IDENTITY"), Map.entry(MessageHeaders.PRODUCE_REGION, "X_PRODUCE_REGION"), Map.entry(MessageHeaders.PRODUCE_TIMESTAMP, "X_PRODUCE_TIMESTAMP") ), List.of("X_", "x_"), - 100, - (5 * 1024 * 1024), + 100, // Maximum header value size in characters + (5 * 1024 * 1024), // Maximum request size (5MB) filterNonCompliantHeaders ); }
35-43: Add parameter documentation for fetchDummyHeaderConfiguration methods.The
filterNonCompliantHeadersparameter isn't explained in the method signatures. Adding Javadoc would clarify its purpose.+/** + * Creates a test header configuration with the specified filter option. + * + * @param filterNonCompliantHeaders If true, headers not starting with allowed prefixes will be filtered out. + * @return A MessageHeaderConfiguration for testing. + */ public static MessageHeaderConfiguration fetchDummyHeaderConfigurationWithParams( boolean filterNonCompliantHeaders ) { return fetchConfiguration(filterNonCompliantHeaders); } +/** + * Creates a default test header configuration that filters out non-compliant headers. + * + * @return A MessageHeaderConfiguration for testing with non-compliant header filtering enabled. + */ public static MessageHeaderConfiguration fetchDummyHeaderConfiguration() { return fetchConfiguration(true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
common/src/main/java/com/flipkart/varadhi/MessageConstants.java(0 hunks)common/src/main/java/com/flipkart/varadhi/utils/MessageHelper.java(0 hunks)common/src/test/java/com/flipkart/varadhi/utils/MessageHelperTest.java(0 hunks)conf/configuration.yml(1 hunks)consumer/src/main/java/com/flipkart/varadhi/consumer/impl/SlidingWindowThrottler.java(1 hunks)consumer/src/main/java/com/flipkart/varadhi/consumer/processing/ProcessingLoop.java(2 hunks)consumer/src/main/java/com/flipkart/varadhi/consumer/processing/UngroupedProcessingLoop.java(2 hunks)consumer/src/test/java/com/flipkart/varadhi/consumer/ConcurrencyAndRLDemo.java(1 hunks)core/src/main/java/module-info.java(1 hunks)entities/build.gradle(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/DlqMessage.java(2 hunks)entities/src/main/java/com/flipkart/varadhi/entities/ProducerMessage.java(3 hunks)entities/src/main/java/com/flipkart/varadhi/entities/StandardHeaders.java(0 hunks)entities/src/main/java/com/flipkart/varadhi/entities/config/MessageHeaderConfiguration.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/constants/MessageHeaders.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/Condition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/AndCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NandCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NorCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NotCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/OrCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ContainsCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/EndsWithCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ExistsCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/InCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/StartsWithCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/utils/HeaderUtils.java(1 hunks)entities/src/main/java/module-info.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/utils/HeaderUtilsTest.java(1 hunks)entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java(1 hunks)entities/src/testFixtures/java/com/flipkart/varadhi/entities/MessageHeaderUtils.java(1 hunks)producer/build.gradle(1 hunks)producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java(4 hunks)pulsar/build.gradle(1 hunks)pulsar/src/main/java/com/flipkart/varadhi/pulsar/entities/PulsarMessage.java(2 hunks)pulsar/src/main/java/com/flipkart/varadhi/pulsar/producer/PulsarProducer.java(2 hunks)pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarMessageTest.java(2 hunks)pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarProducerTest.java(5 hunks)server/src/main/java/com/flipkart/varadhi/config/AppConfiguration.java(2 hunks)server/src/main/java/com/flipkart/varadhi/config/MessageHeaderConfiguration.java(0 hunks)server/src/main/java/com/flipkart/varadhi/utils/HeaderUtils.java(0 hunks)server/src/main/java/com/flipkart/varadhi/verticles/webserver/WebServerVerticle.java(3 hunks)server/src/main/java/com/flipkart/varadhi/web/Extensions.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/RequestTelemetryConfigurator.java(2 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/produce/HeaderValidationHandler.java(0 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/produce/PreProduceHandler.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/produce/ProduceHandlers.java(6 hunks)server/src/test/java/com/flipkart/varadhi/config/MessageHeaderConfigurationTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/utils/HeaderUtilsTest.java(0 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java(3 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java(3 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java(9 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/ProduceTestBase.java(2 hunks)
💤 Files with no reviewable changes (8)
- entities/src/main/java/com/flipkart/varadhi/entities/StandardHeaders.java
- server/src/main/java/com/flipkart/varadhi/utils/HeaderUtils.java
- common/src/test/java/com/flipkart/varadhi/utils/MessageHelperTest.java
- server/src/main/java/com/flipkart/varadhi/config/MessageHeaderConfiguration.java
- server/src/test/java/com/flipkart/varadhi/utils/HeaderUtilsTest.java
- server/src/main/java/com/flipkart/varadhi/web/v1/produce/HeaderValidationHandler.java
- common/src/main/java/com/flipkart/varadhi/utils/MessageHelper.java
- common/src/main/java/com/flipkart/varadhi/MessageConstants.java
✅ Files skipped from review due to trivial changes (4)
- consumer/src/test/java/com/flipkart/varadhi/consumer/ConcurrencyAndRLDemo.java
- consumer/src/main/java/com/flipkart/varadhi/consumer/impl/SlidingWindowThrottler.java
- server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java
- server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java
🧰 Additional context used
🪛 GitHub Actions: Build Docker Image
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/InCondition.java
[error] 1-1: Spotless formatting check failed. The file has format violations.
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java
[error] 1-1: Spotless formatting check failed. The file has format violations.
🪛 GitHub Actions: E2E
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/InCondition.java
[error] 1-1: Spotless formatting check failed. The file has format violations.
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java
[error] 1-1: Spotless formatting check failed. The file has format violations.
🔇 Additional comments (107)
entities/build.gradle (2)
12-12: Dependency Addition: Apache Commons JEXL
The addition of the Apache Commons JEXL dependency (org.apache.commons:commons-jexl3:3.4.0) appears well-integrated into the project. Please verify that version 3.4.0 is compatible with your current dependency ecosystem and that there are no transitive conflicts.
13-13: Test Dependency Addition: Common Module
The new test dependency (testImplementation project(path: ':common')) is properly added for supporting testing of the header utility functionalities. Ensure that the common module only contains test-related utilities to avoid any accidental coupling with production code.server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java (2)
5-6: Added imports for new header management approach.These new imports align with the PR goal of introducing a new header management infrastructure, replacing the legacy message header classes.
167-168: Updated header retrieval using the new HeaderUtils approach.The code now uses
HeaderUtils.getHeader(MessageHeaders.MSG_ID)andHeaderUtils.getHeader(MessageHeaders.GROUP_ID)instead of direct static constants, which aligns with the broader refactoring to use the new header management infrastructure. This approach provides better encapsulation and maintainability.pulsar/src/main/java/com/flipkart/varadhi/pulsar/producer/PulsarProducer.java (3)
5-6: Good addition of new header management imports.These imports align with the new header management infrastructure being introduced, replacing the legacy message header classes.
87-88: Correctly migrated from StandardHeaders to the new header management system.The implementation properly transitions from using the legacy
StandardHeaders.GROUP_IDstatic constant to the new approach withHeaderUtils.getHeader(MessageHeaders.GROUP_ID). This change enhances maintainability and type safety through the use of enums rather than string constants.
53-55: Consider addressing TODOs as part of feature work.These TODOs regarding the max pending messages configuration have been left in place. Since you're already working on improvements to the messaging infrastructure, it might be a good opportunity to address these items.
Are these TODOs still relevant with the new header management system? If you decide to keep them for a future task, consider creating tracking issues to ensure they don't get forgotten.
pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarMessageTest.java (3)
4-8: Import changes reflect migration to new header management system.These new imports align with the PR's objective to replace legacy message header classes with a new header management infrastructure.
27-28: Properly initializes the new header management system.The initialization of
MessageHeaderConfigurationandHeaderUtilsis necessary for the new header management approach and follows the correct sequence.
30-31: Updated header access to use the new HeaderUtils API.The code correctly migrates from direct references to header constants (likely from
StandardHeaders) to the new pattern usingHeaderUtils.getHeader(MessageHeaders.XXX).consumer/src/main/java/com/flipkart/varadhi/consumer/processing/ProcessingLoop.java (2)
8-9: Import changes align with the new header management approach.The imports have been updated to use the new utility classes for header management, which is consistent with the PR objective of introducing a new header management infrastructure.
146-146:✅ Verification successful
Verify the nested getHeader calls.
There appears to be a nested call to
getHeader()- firstHeaderUtils.getHeader(MessageHeaders.MSG_ID)returns a header key, which is then passed tomsg.getMessage().getHeader(). This nesting seems redundant or could be a potential bug. Consider simplifying this to a single call if possible.
🏁 Script executed:
#!/bin/bash # Check HeaderUtils implementation to understand if this nesting is necessary # First, find the HeaderUtils class to see its implementation fd -t f -e java "HeaderUtils.java" --exec cat {} # Also check the Message interface or class to see the getHeader method fd -t f -e java "Message.java" --exec cat {}Length of output: 17231
Verification Complete – Nested
getHeaderCalls Are Working as IntendedAfter reviewing the HeaderUtils and Message implementations, the nested calls are part of the intended design. The call to
HeaderUtils.getHeader(MessageHeaders.MSG_ID)converts the enum constant into the corresponding string header key (e.g., "X_MESSAGE_ID"), which is then used bymsg.getMessage().getHeader()to fetch the header value. This pattern is consistent in other parts of the codebase (for example, in the implementations ofPulsarMessage,DlqMessage, andProducerMessage) and supports maintainability through centralized header configuration.No changes are needed in this area.
entities/src/main/java/com/flipkart/varadhi/entities/DlqMessage.java (2)
6-7: Import changes reflect new header management approach.These new imports align with the PR objective of introducing a new header management infrastructure.
29-35:✅ Verification successful
Verify consistent header access across codebase.
The switch from direct header key references to using
HeaderUtils.getHeader(MessageHeaders.XXX)needs to be consistently applied across the codebase to avoid any potential mismatches in header key names.
🏁 Script executed:
#!/bin/bash # Check for any remaining direct references to old StandardHeaders class echo "Checking for any remaining references to StandardHeaders..." rg "StandardHeaders" --type java # Check for consistent usage of MessageHeaders enum with HeaderUtils echo "Checking for consistent header access patterns..." rg "getHeader\(.*MessageHeaders" --type javaLength of output: 9328
Consistent header access confirmed across the codebase.
All instances, including those inentities/src/main/java/com/flipkart/varadhi/entities/DlqMessage.java, uniformly useHeaderUtils.getHeader(MessageHeaders.XXX), and no remaining references toStandardHeaderswere found. Please continue using this standardized approach for any header access to ensure consistency.pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarProducerTest.java (8)
6-7: Imports updated to use new header management utilitiesThe imports reflect the transition to the new header management infrastructure by including
HeaderUtilsandMessageHeadersclasses, which replace the legacy header management classes.
59-59: Header configuration initialization added to preTestGood addition of
HeaderUtils.initialize()in the test setup to ensure the header configuration is properly initialized before running tests. This is a required step when using the new header management approach.
156-156: Updated to use HeaderUtils for accessing GROUP_ID headerThe code has been modified to use
HeaderUtils.getHeader(MessageHeaders.GROUP_ID)instead of direct access to header constants, which is consistent with the new header management approach.
162-162: Consistent header removal using HeaderUtilsHeader removal is now also updated to use the
HeaderUtils.getHeader(MessageHeaders.GROUP_ID)utility method, maintaining consistency with the new approach.
169-169: Consistent header setting in second test caseAnother instance of the header update pattern, maintaining consistency across the test file.
185-185: HeaderUtils used consistently in testMessagePropertiesThe
testMessagePropertiesmethod has also been updated to use the new header management approach for adding the GROUP_ID header.
196-196: Consistent assertion using HeaderUtils for GROUP_IDThe assertion for verifying the GROUP_ID header in properties has been updated to use
HeaderUtils.getHeader(MessageHeaders.GROUP_ID), maintaining consistency with the rest of the changes.
1-234:✅ Verification successful
Verify test coverage for HeaderUtils initialization
The test properly initializes HeaderUtils, but it would be good to verify that there are tests covering error scenarios such as missing header configurations or initialization failures.
🏁 Script executed:
#!/bin/bash # Description: Check for tests that verify HeaderUtils error handling # Search for tests related to HeaderUtils initialization failures echo "Searching for tests related to HeaderUtils initialization failures..." rg -A 3 -B 3 "HeaderUtils.*initialize" --type java --glob "**/*Test.java" # Search for tests that might be checking for missing configurations echo "Searching for tests that verify behavior with missing header configurations..." rg -A 3 -B 3 "fetchDummyHeaderConfiguration|missing.*header|HeaderUtils.*null" --type java --glob "**/*Test.java"Length of output: 17829
HeaderUtils Error Scenario Coverage Verified
The investigations confirm that error scenarios—such as missing header configurations and initialization failures—are already validated in tests like
testMissingRequiredHeaders()withinHeaderUtilsTest.java. Given that these error cases are comprehensively covered in the dedicated tests, no additional error handling tests are necessary inPulsarProducerTest.java.entities/src/main/java/com/flipkart/varadhi/entities/ProducerMessage.java (5)
3-4: Good addition of new header management utilities.The addition of these imports supports the broader refactoring effort to standardize header management across the codebase. Using dedicated utility classes for header management improves code organization and maintainability.
28-28: Header access changed to use new utility methods.Similar to the
getMessageId()method, this change adopts the new header management approach for consistent header access across the application.
38-38:❓ Verification inconclusive
Improved list element access using getFirst().
Changed from
requestHeaders.get(key).get(0)torequestHeaders.get(key).getFirst(), which is more semantically clear. However, ensure thatgetFirst()handles empty lists appropriately to avoid potentialNoSuchElementException.
🏁 Script executed:
#!/bin/bash # Check if getFirst() is a custom method or extension and how it handles empty lists rg -A 2 "getFirst" --type javaLength of output: 3558
Action Required: Verify safe handling of empty lists with getFirst()
The change from using
requestHeaders.get(key).get(0)torequestHeaders.get(key).getFirst()does improve semantic clarity. However, please confirm that the customgetFirst()implementation properly handles empty lists (e.g., by returningnullor a safe default) to prevent a potentialNoSuchElementException. This method is used in several parts of the codebase, so ensuring its robustness is essential.
23-23:❓ Verification inconclusive
Header access changed to use new utility methods.
The method now uses
HeaderUtils.getHeader(MessageHeaders.MSG_ID)instead of directly referencingStandardHeaders. This change aligns with the new header management infrastructure while maintaining the same functionality.
🏁 Script executed:
#!/bin/bash # Verify that HeaderUtils.getHeader is correctly implemented and returns the expected values ast-grep --pattern $'class HeaderUtils { $$$ public static String getHeader(MessageHeaders header) { $$$ } $$$ }'Length of output: 126
Please manually verify the HeaderUtils#getHeader implementation for correctness.
- In entities/src/main/java/com/flipkart/varadhi/entities/ProducerMessage.java (line 23), the header retrieval logic was updated to use
HeaderUtils.getHeader(MessageHeaders.MSG_ID)rather than accessingStandardHeadersdirectly.- This change is meant to align with the new header management infrastructure while maintaining existing functionality.
- However, the automated AST search did not yield any output regarding the implementation of
HeaderUtils.getHeader. Please double-check manually that this method is correctly defined and behaves as expected.
20-20:✅ Verification successful
Address the existing TODO comment.
There's an existing TODO comment indicating that changes to
getMessageId()may affect JSON serialization. Since this method has been modified, please verify if this TODO is still relevant or if it has been addressed with the new header management implementation.
🏁 Script executed:
#!/bin/bash # Check for JSON serialization related to MessageId rg -A 5 "MessageId.*json|json.*MessageId" --type javaLength of output: 1613
Modify or Remove TODO Comment on JSON Handling
The custom header management implementation now leverages dedicated JSON serialization/deserialization (as seen in the pulsar module’s MessageIdSerializer and MessageIdDeserializer), which appears to fully address the previous concerns regarding JSON output from getMessageId(). Please confirm that the new implementation covers all required scenarios for JSON serialization. If it does, kindly remove the TODO comment from ProducerMessage.java.
server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java (2)
4-6: Proper migration to new header management infrastructure.The imports for the new header utilities align with the PR objective of migrating from legacy header classes to the new infrastructure.
41-41: Added appropriate HeaderUtils initialization.Good addition of HeaderUtils initialization with dummy configuration for testing purposes. This setup ensures the test environment is properly configured for the new header management system.
pulsar/build.gradle (1)
11-12: Good addition of entity test fixtures dependency.This addition allows tests in the pulsar module to utilize test fixtures from the entities project, which aligns with the PR's focus on implementing new header management infrastructure.
producer/build.gradle (1)
14-15: Appropriate test dependency addition.Adding the entities test fixtures dependency is a good approach that enables the producer module's tests to utilize the shared test resources from the entities project, particularly for testing the new header management functionality.
core/src/main/java/module-info.java (1)
7-9:❓ Verification inconclusive
Verify the need for all new module dependencies.
The addition of these three module requirements (
jakarta.validation,io.vertx.core, andcom.google.common) expands the core module's dependencies. Ensure that all these dependencies are actually utilized within the core module and not just referenced transitively.
🏁 Script executed:
#!/bin/bash # Check if these new dependencies are actually used in core module echo "Checking for jakarta.validation usage in core module:" rg -l "import\s+jakarta\.validation" core/src/main/java echo -e "\nChecking for io.vertx.core usage in core module:" rg -l "import\s+io\.vertx\.core" core/src/main/java echo -e "\nChecking for com.google.common usage in core module:" rg -l "import\s+com\.google\.common" core/src/main/javaLength of output: 520
Action: Confirm Dependency Usage in core/src/main/java/module-info.java
The automated search did not reveal any direct import usage ofjakarta.validation,io.vertx.core, orcom.google.commonwithin the core module. Please manually verify that these dependencies are indeed needed and are not merely being brought in transitively. This manual check is critical to avoid unnecessarily expanding the module's dependency footprint.entities/src/main/java/com/flipkart/varadhi/entities/constants/MessageHeaders.java (1)
5-32: Well-structured enum for header constants.The implementation of the
MessageHeadersenum is a strong improvement over the previous approach using separate classes likeStandardHeadersandMessageConstants. Using an enum provides several benefits:
- Type safety when referencing headers
- Centralized definition of all message headers
- Proper namespace isolation
- Better IDE support with autocompletion
The
@JsonPropertyannotations are correctly applied to maintain serialization/deserialization compatibility.server/src/main/java/com/flipkart/varadhi/web/Extensions.java (2)
29-30: Constant relocation from removed MessageConstants class.The constant
ANONYMOUS_IDENTITYhas been moved here from the deletedMessageConstantsclass. This is consistent with the overall PR objective of refactoring header management infrastructure.
170-173:✅ Verification successful
Verify usages of getIdentityOrDefault method.
Since the
ANONYMOUS_IDENTITYconstant has been relocated, make sure all callers of this method are using the updated import path.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining imports of MessageConstants.ANONYMOUS_IDENTITY echo "Checking for any remaining imports of MessageConstants.ANONYMOUS_IDENTITY:" rg -l "import.*MessageConstants.*ANONYMOUS_IDENTITY" || echo "No more imports of MessageConstants.ANONYMOUS_IDENTITY found." echo "Checking uses of ANONYMOUS_IDENTITY:" rg "ANONYMOUS_IDENTITY" --type javaLength of output: 1022
Verification Complete: All usages of ANONYMOUS_IDENTITY now reference the correct import.
- No references to the old constant (
MessageConstants.ANONYMOUS_IDENTITY) remain in the codebase.- The test and main classes correctly import
ANONYMOUS_IDENTITYfromcom.flipkart.varadhi.web.Extensions.- Callers of the
getIdentityOrDefaultmethod are, therefore, aligned with the updated relocation.entities/src/main/java/module-info.java (2)
14-15: New packages exported to support header management refactoring.The addition of these exports aligns with the PR objective of introducing a new header management infrastructure. Make sure classes in these packages have proper documentation and are designed for public API use.
8-8:❓ Verification inconclusive
Verify the necessity of the Apache Commons JEXL3 dependency.
The addition of the JEXL3 dependency suggests new expression language functionality. Ensure this dependency is being properly utilized in the filter implementation.
🏁 Script executed:
#!/bin/bash # Description: Check for usage of JEXL3 in the codebase to verify if it's being used echo "Checking for imports of org.apache.commons.jexl3:" rg -l "import org.apache.commons.jexl3" || echo "No direct imports of org.apache.commons.jexl3 found." # Check for any references to JEXL in the code echo "Checking for references to JEXL in the code:" rg "jexl|JEXL" --type java || echo "No references to JEXL found in Java code."Length of output: 472
JEXL3 Dependency Usage Check
The search confirms that aside from the dependency declaration in the module descriptor, there aren’t any direct imports or references to Apache Commons JEXL3 in the code (including the filter implementation). Please verify whether this dependency is intended for dynamic expression evaluation (perhaps via reflection or future enhancements) or if it can be removed to avoid unnecessary bloat.
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ExistsCondition.java (2)
16-18: LGTM: Method is concise and correctly implements existence check.The evaluate method correctly checks if the headers contain the given key. It's simple and efficient.
20-23: LGTM: Clear and informative toString implementation.The toString method provides a useful representation for debugging and logging purposes.
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/Condition.java (3)
1-14: Well-structured interface definition with clear documentation.The interface definition is well-documented with comprehensive JavaDoc that clearly explains the purpose of the Condition interface and its intended usage.
15-26: Good use of Jackson annotations for polymorphic type handling.The implementation correctly uses
@JsonTypeInfoand@JsonSubTypesannotations to support polymorphic serialization and deserialization. This will allow the deserializer to correctly instantiate the appropriate implementation class based on the "op" property.
27-43: Interface design follows good practices.The interface is cohesive and follows the Single Responsibility Principle with clear method signatures. The
evaluatemethod takes aMultimap<String, String>which is appropriate for header evaluation as headers can have multiple values.pulsar/src/main/java/com/flipkart/varadhi/pulsar/entities/PulsarMessage.java (3)
3-4: Appropriate imports added for the header management utilities.Good replacement of legacy header management with the new imports for
HeaderUtilsandMessageHeaders.
68-70: Header access correctly migrated to new utility approach.The change from direct header constant reference to utility method call improves encapsulation and consistency.
73-75: Header access correctly migrated to new utility approach.Similar to the message ID change, this correctly updates the group ID header access to use the new utility method.
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/InCondition.java (3)
1-11: Appropriate imports and package declaration.All necessary imports are included for JSON processing, validation, and condition functionality.
🧰 Tools
🪛 GitHub Actions: Build Docker Image
[error] 1-1: Spotless formatting check failed. The file has format violations.
🪛 GitHub Actions: E2E
[error] 1-1: Spotless formatting check failed. The file has format violations.
19-22: Elegant implementation of the evaluate method.The evaluate method uses Java streams efficiently to check if any of the header values for the specified key are contained in the provided list of values.
24-29: Well-formatted toString method for readable output.The toString method provides a clear string representation of the condition, properly formatting the values list.
consumer/src/main/java/com/flipkart/varadhi/consumer/processing/UngroupedProcessingLoop.java (2)
9-10: Appropriate imports added for the header management utilities.Good replacement of legacy header management with the new imports for
HeaderUtilsandMessageHeaders.
79-83: Header access correctly migrated to new utility approach.The change from direct header constant reference to utility method call improves consistency across the codebase and aligns with the new header management approach.
server/src/main/java/com/flipkart/varadhi/verticles/webserver/WebServerVerticle.java (2)
33-33: Refactoring to use PreProduceHandlerThe code is importing the new
PreProduceHandlerclass instead of the removedHeaderValidationHandler, aligning with the PR's goal of introducing a new header management infrastructure.
245-245: Implementation of PreProduceHandler looks goodThe code now instantiates
PreProduceHandlerinstead of the previousHeaderValidationHandler, which is consistent with the overall header management refactoring.entities/src/main/java/com/flipkart/varadhi/entities/utils/HeaderUtils.java (3)
20-33: Synchronized initialization looks goodThe synchronized initialization method with checks to prevent re-initialization is well-implemented. It handles null configuration appropriately and ensures thread safety.
39-48: Good enforcement of initialization before usageThe implementation correctly checks if the class is initialized before retrieving header values, which prevents potential null pointer exceptions or incorrect behavior.
62-78: Efficient header filtering implementationThe implementation efficiently filters headers based on configured prefixes and transforms keys to uppercase for standardization. This should improve consistency in header handling across the application.
server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java (2)
11-11: New import for HeaderUtilsThe code now imports the new HeaderUtils class, which is consistent with the overall header management refactoring.
43-43: Proper initialization of HeaderUtils in test setupThe test correctly initializes HeaderUtils with a dummy configuration before each test execution, ensuring that header-related functionality will work properly in the tests.
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ContainsCondition.java (2)
8-15: Well-structured record implementation with proper JSON annotationsThe ContainsCondition is properly implemented as a Java record with appropriate JSON annotations for deserialization. The required properties are clearly marked.
22-25: Clear toString implementationThe toString method provides a clear, human-readable representation of the condition, which is helpful for debugging and logging.
server/src/main/java/com/flipkart/varadhi/web/RequestTelemetryConfigurator.java (1)
3-4: Appropriate imports for the new header management approach.The imports correctly bring in the new header management infrastructure components that replace the removed
StandardHeadersclass.entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/StartsWithCondition.java (2)
9-14: Well-structured record implementation for string prefix matching.Good use of Java record for an immutable condition implementation with required parameter validation.
21-24: Clear string representation format.The toString implementation provides a readable representation of the condition, which is helpful for logging and debugging.
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/OrCondition.java (3)
14-19: Well-structured class design for logical OR operator.The class correctly implements the Condition interface with appropriate Lombok annotations and a properly typed field for storing multiple conditions.
26-29: Efficient string representation.Good use of Stream API to create a clean string representation of the OR condition, joining individual condition strings with the "or" operator.
31-34: Efficient evaluation using Stream API.The implementation correctly uses
anyMatch()to implement logical OR semantics - returning true if any of the conditions evaluates to true.However, consider adding a check for empty values collection to handle edge cases gracefully:
@Override public boolean evaluate(Multimap<String, String> headers) { + if (values == null || values.isEmpty()) { + return false; // An empty OR condition evaluates to false + } return values.stream().anyMatch(condition -> condition.evaluate(headers)); }entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/AndCondition.java (3)
1-13: Imports and package structure look goodThe imports are well organized and include all necessary dependencies for the AndCondition class implementation.
26-29: String representation is well implementedThe toString method effectively uses Java streams to create a readable string representation of the AND condition.
31-34: Evaluation logic is correctly implementedThe evaluate method properly implements AND logic using allMatch on the stream of conditions.
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NandCondition.java (3)
1-13: Imports and package structure look goodThe imports are well organized and include all necessary dependencies for the NandCondition class implementation.
26-29: String representation is well implementedThe toString method effectively represents the NAND condition as "not(condition1 and condition2...)", which clearly conveys the logical operation.
31-34: Evaluation logic is correctly implementedThe evaluate method properly implements NAND logic by negating the result of allMatch on the conditions.
server/src/main/java/com/flipkart/varadhi/config/AppConfiguration.java (2)
5-6: New imports for header managementThese imports support the transition to the new header management system, which is consistent with the PR objective of adding feature filter support.
67-67: MessageHeaderConfiguration field is properly annotatedThe field is correctly annotated with @NotNull and @Valid to ensure proper validation.
entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NorCondition.java (3)
1-13: Imports and package structure look goodThe imports are well organized and include all necessary dependencies for the NorCondition class implementation.
26-29: String representation is well implementedThe toString method effectively represents the NOR condition as "not(condition1 or condition2...)", which clearly conveys the logical operation.
31-34: Evaluation logic is correctly implementedThe evaluate method properly implements NOR logic using noneMatch on the stream of conditions, returning true only if none of the conditions evaluate to true.
entities/src/main/java/com/flipkart/varadhi/entities/config/MessageHeaderConfiguration.java (2)
13-28: Record definition and constructor look good.The Java record is a good choice for this immutable configuration data. The explicit constructor with JsonProperty annotations ensures proper deserialization from the YAML configuration.
30-50:❓ Verification inconclusive
Validation implementation has a potential issue with missing headers.
The current validation logic correctly checks allowed prefixes and that existing header values are valid. However, it assumes that all
MessageHeadersenum values will have a corresponding entry in the mapping.
🏁 Script executed:
#!/bin/bash # Look for the MessageHeaders enum to understand all possible values fd --type f --exec grep -l "enum MessageHeaders" {}Length of output: 140
Attention: Validate Handling of Missing Header Mappings
After verifying that the
MessageHeadersenum is defined inentities/src/main/java/com/flipkart/varadhi/entities/constants/MessageHeaders.java, it’s clear that the validation method inMessageHeaderConfiguration:
- Iterates over all enum values.
- Retrieves header values from the mapping.
- Throws an exception if a value is
null, empty, or does not start with a valid prefix.This design assumes that every header declared in
MessageHeadersmust have an entry in the mapping. If an entry is missing, the logic will trigger an exception, which may or may not be the intended behavior.Suggestions:
- Explicit Missing Check: Consider adding an explicit check that clearly distinguishes between a missing mapping entry and an invalid value.
- Documentation: If the current approach is by design, document this assumption to clarify that missing headers are not allowed and will result in an error.
Please verify that this behavior aligns with the intended design and review the impact of these suggestions on the overall codebase.
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java (2)
134-144: Good practice testing invalid conditions.The test for malformed conditions is a good practice to ensure proper error handling and validation.
1-144:❓ Verification inconclusive
Fix Spotless formatting issues.
The pipeline failures indicate Spotless formatting issues that need to be fixed.
🏁 Script executed:
#!/bin/bash # Run Spotless formatting check if command -v ./gradlew &> /dev/null; then ./gradlew spotlessCheck else echo "Gradle wrapper not found, cannot check formatting" fiLength of output: 1254
Attention: The Spotless check isn’t reliably indicating formatting issues because the Gradle build is failing due to a Java toolchain configuration error. The log shows that the toolchain at
/usr/lib/jvm/java-17-openjdk-amd64doesn’t provide the requiredJAVA_COMPILERcapability needed by the build script inbuildSrc. This setup issue is preventing the formatting check from running successfully.Please address the following before re-running the Spotless check:
- Java Toolchain Configuration: Update or adjust the build’s Java toolchain settings to ensure that a valid Java compiler with the required capabilities is available.
- Re-run Spotless Check: Once the toolchain issue is resolved, run the Spotless check (e.g., with
./gradlew spotlessCheck) to verify that code formatting complies with project guidelines.- Verify Formatting Issues: Confirm whether any additional formatting corrections are needed after resolving the build environment issue.
🧰 Tools
🪛 GitHub Actions: Build Docker Image
[error] 1-1: Spotless formatting check failed. The file has format violations.
🪛 GitHub Actions: E2E
[error] 1-1: Spotless formatting check failed. The file has format violations.
producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java (1)
291-294: Updated header access looks good.The updated code correctly uses the new
HeaderUtils.getHeader()method with the appropriateMessageHeadersenum values.entities/src/test/java/com/flipkart/varadhi/utils/HeaderUtilsTest.java (1)
140-156: LGTM! Comprehensive test for header filtering behavior.This test effectively validates the filtering behavior for non-compliant headers based on the configuration parameter. The clear structure with separate assertions and descriptive failure messages makes it easy to understand.
server/src/main/java/com/flipkart/varadhi/web/v1/produce/PreProduceHandler.java (2)
65-69: LGTM! Clear method for identifying ID headers.The
isIdHeadermethod provides a clean abstraction for checking whether a header is of ID type, making the code more maintainable.
51-52:❓ Verification inconclusive
Character encoding assumption might lead to inaccurate size calculation.
The calculation assumes 2 bytes per character which is valid for UTF-16, but may be incorrect for other encodings. Consider using the actual byte length of the strings with the correct encoding.
🏁 Script executed:
#!/bin/bash # Check if there's a specific character encoding configured or used in the application # Look for encoding configurations in relevant properties/config files echo "Searching for encoding configurations..." rg -i "charset|encoding" --type properties --type xml --type json --type yaml # Check if there are explicit encoding operations in the code echo "Checking for explicit encoding operations in the code..." ast-grep --pattern $'String.getBytes($encoding)'Length of output: 378
Action Required: Revisit Encoding Assumption in Byte Length Calculation
After verifying the repository, we couldn’t locate any explicit encoding configurations or explicit invocations of methods like
String.getBytes(encoding). This confirms that the current logic inserver/src/main/java/com/flipkart/varadhi/web/v1/produce/PreProduceHandler.java(lines 51–52):int byteLength = (key.length() + value.length()) * 2; headersAndBodySize += byteLength;relies on the implicit assumption that every character uses exactly 2 bytes (UTF-16). While this may be true for Java’s in-memory representation, it might not yield the correct size when the strings are encoded for transmission (e.g., using UTF-8).
Recommendations:
- Confirm the expected encoding for these strings.
- If a different encoding (such as UTF-8) is used when transmitting data, calculate the actual byte length using something like:
int byteLength = key.getBytes(StandardCharsets.UTF_8).length + value.getBytes(StandardCharsets.UTF_8).length;- Alternatively, if UTF-16 is indeed the intended encoding, document this assumption explicitly to avoid confusion.
server/src/test/java/com/flipkart/varadhi/config/MessageHeaderConfigurationTest.java (3)
3-5: Import structure matches the new entity organization.The imports accurately reflect the shift to using the new
MessageHeadersenum from the constants package and the structuredMessageHeaderConfigurationclass.
25-36: Validation approach refactored to use functional interface.The test has been refactored to use a
Runnableinstead ofExecutablefor validation, which aligns with the functional programming paradigm and simplifies the test structure.
46-69: Improved test utility method using the new header mapping structure.The refactored
getDefaultMessageHeaderConfigmethod now directly constructs aMessageHeaderConfigurationusing entries mapped from theMessageHeadersenum to their string values, which better represents the production code's structure and improves test maintainability.server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java (7)
3-10: Updated imports to reflect the new header management framework.The imports have been appropriately updated to include the new
MessageHeaderUtils,HeaderUtils, andMessageHeadersclasses from their respective packages.
28-32: Class structure updated for new pre-produce handling approach.The class has been renamed from
HeaderValidationTesttoPreProduceHandlerTestand the handler type has been changed toPreProduceHandler, reflecting the shift from simple header validation to comprehensive pre-produce handling.
33-46: Setup method initializes the header utilities for testing.The
PreTestmethod now properly initializes the header utilities usingHeaderUtils.initialize()with dummy configuration, ensuring headers are consistently managed throughout the tests.
56-58: Updated header names to use the new naming convention.The test now correctly uses the
X_MESSAGE_IDandX_ForwardedForheader names, which align with the new header naming convention implemented in the system.
69-80: Test case updated for header size validation.The test for validating maximum header key size has been updated to use the new header utility and appropriate error message format.
82-99: New test added for request body size limitation.A new test has been added to verify that requests with bodies exceeding the maximum allowed size (5MB) are rejected, which is an important validation for preventing DoS attacks.
102-109: Multi-value header test updated to use HeaderUtils.The test for multi-value headers now uses the new
HeaderUtils.getHeader()method to retrieve header names from theMessageHeadersenum, ensuring consistent header naming throughout the test.server/src/test/java/com/flipkart/varadhi/web/produce/ProduceTestBase.java (3)
5-7: Import additions for new header utilities.The imports have been updated to include
MessageHeaderUtilsandHeaderUtils, which are essential for the new header management approach.
52-55: Header utilities initialization added to setup.Header utilities are now properly initialized in the setup method, ensuring consistent header management across tests.
57-63: ProduceHandlers constructor updated with new handler and region parameter.The
ProduceHandlersconstructor now correctly uses thepreProduceHandler::validatemethod reference and includes thedeployedRegionparameter, aligning with the updated class structure.server/src/main/java/com/flipkart/varadhi/web/v1/produce/ProduceHandlers.java (10)
5-7: Import additions for new header management classes.The imports have been updated to include
HeaderUtilsandMessageHeadersfrom their respective packages, supporting the new header management framework.
12-13: Updated to use Extensions class for utility methods.The import of
Extensionsclass replaces the need for direct references to constants likeANONYMOUS_IDENTITYthat were previously inMessageConstants.
20-21: Added ArrayListMultimap for flexible header management.Using
ArrayListMultimapallows for more flexible and structured handling of headers with multiple values.
25-28: Added Lombok annotations for cleaner code.The
@AllArgsConstructorannotation eliminates the need for a manually written constructor, reducing boilerplate code.
44-46: Class annotations updated for improved functionality.Added
@ExtensionMethodto use extension methods fromExtensionsclass and@AllArgsConstructorto generate a constructor, improving code readability and reducing boilerplate.
47-52: Class fields updated to reflect new header management approach.The
headerValidationHandlerfield has been replaced withpreProduceHandler, aligning with the broader refactoring to enhance header management.
63-64: Route definition updated to use preProduceHandler.The route definition now correctly uses
preProduceHandlerfor validation, reflecting the new pre-produce handling approach.
84-85: Simplified identity retrieval.The code now uses
ctx.getIdentityOrDefault()instead of checking ifctx.user()is null, which is cleaner and leverages the extension method.
95-96: Updated method call to pass the routing context.The
buildMessageToProducemethod call now correctly passes the routing context instead of extracting the produce identity beforehand.
137-156: Comprehensive refactoring of header handling.The
buildMessageToProducemethod has been significantly improved to:
- Filter headers that don't follow semantic rules using
HeaderUtils.returnVaradhiRecognizedHeaders- Enrich the headers with system information (identity, region, timestamp)
- Use the utility methods for consistent header naming
This approach centralizes header management, improves maintainability, and ensures consistency across the codebase.
server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/EndsWithCondition.java
Outdated
Show resolved
Hide resolved
...ties/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/InCondition.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/verticles/webserver/WebServerVerticle.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ContainsCondition.java
Outdated
Show resolved
Hide resolved
.../main/java/com/flipkart/varadhi/entities/filters/conditions/logical/StartsWithCondition.java
Outdated
Show resolved
Hide resolved
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java
Outdated
Show resolved
Hide resolved
producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
conf/configuration.yml(0 hunks)entities/build.gradle(1 hunks)producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java(1 hunks)pulsar/src/main/java/com/flipkart/varadhi/pulsar/producer/PulsarProducer.java(1 hunks)pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarMessageTest.java(1 hunks)server/src/main/java/com/flipkart/varadhi/config/AppConfiguration.java(1 hunks)server/src/main/java/com/flipkart/varadhi/verticles/webserver/WebServerVerticle.java(2 hunks)server/src/main/java/com/flipkart/varadhi/web/RequestTelemetryConfigurator.java(0 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/produce/PreProduceHandler.java(1 hunks)server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(0 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java(2 hunks)
💤 Files with no reviewable changes (3)
- server/src/main/java/com/flipkart/varadhi/web/RequestTelemetryConfigurator.java
- conf/configuration.yml
- server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java
✅ Files skipped from review due to trivial changes (6)
- pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarMessageTest.java
- server/src/main/java/com/flipkart/varadhi/config/AppConfiguration.java
- server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java
- producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java
- server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java
- server/src/main/java/com/flipkart/varadhi/web/v1/produce/PreProduceHandler.java
🚧 Files skipped from review as they are similar to previous changes (5)
- entities/build.gradle
- server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java
- pulsar/src/main/java/com/flipkart/varadhi/pulsar/producer/PulsarProducer.java
- server/src/main/java/com/flipkart/varadhi/verticles/webserver/WebServerVerticle.java
- server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java
🔇 Additional comments (1)
server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java (1)
53-54: Good use of standardized headers!Using
StdHeaders.get()instead of hardcoded string literals is a good practice. This change improves code maintainability by centralizing header key definitions and reducing the chance of typos.
server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java (1)
1-1:⚠️ Potential issuePackage declaration needs to be corrected.
The package declaration should reflect the full package path to match Flipkart's package structure.
-package filters; +package com.flipkart.varadhi.entities.filters;
🧹 Nitpick comments (4)
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java (4)
19-19: Fix typo in class name.The class name has a typo in "Deserialze" which should be "Deserialize".
-public class FilterDeserialzeAndEvaluatorTest { +public class FilterDeserializeAndEvaluatorTest {
132-135: Simplify the test assertion.Since you're working with a list that will likely have only one condition per test case (based on your test data), the loop is unnecessary and could be simplified.
- for (int i = 0; i < conditions.size(); i++) { - Condition condition = conditions.get(i); - assertEquals(expectedToString, condition.toString()); - } + assertEquals(expectedToString, conditions.get(0).toString());If you're expecting multiple conditions in some test cases, consider parameterizing the expected strings accordingly.
21-92: Consider organizing test data with constants.The test data contains many hardcoded strings that would be more maintainable if extracted into constants, especially the operation types.
Example approach:
private static final String OP_AND = "AND"; private static final String OP_OR = "OR"; private static final String OP_STARTS_WITH = "startsWith"; // ... more constants private static Stream<Arguments> provideConditions() { return Stream.of( Arguments.of( String.format("[{\"op\":\"%s\",\"values\":[{\"op\":\"%s\",\"key\":\"X_abc\",\"value\":\"my_prefix\"},...", OP_AND, OP_STARTS_WITH), false, "startsWith(X_abc,\"my_prefix\") and endsWith(X_abc,\"suffix\")" ), // ... other test cases ); }
113-117: Consider extracting header setup to a helper method.The headers setup could be extracted to a helper method for better maintainability, especially if other test methods need the same setup.
private Multimap<String, String> createTestHeaders() { Multimap<String, String> headers = ArrayListMultimap.create(); headers.put("X_abc", "my_prefix"); headers.put("X_abc", "my_prefix1"); headers.put("X_abc", "my_prefix2"); return headers; }Then use it in your test method:
Multimap<String, String> headers = createTestHeaders();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
entities/build.gradle(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/config/MessageHeaderConfiguration.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/AndCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NandCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NorCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NotCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/OrCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ContainsCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/EndsWithCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ExistsCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/InCondition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/StartsWithCondition.java(1 hunks)entities/src/main/java/module-info.java(1 hunks)entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java(1 hunks)server/src/main/java/com/flipkart/varadhi/config/AppConfiguration.java(0 hunks)server/src/main/java/com/flipkart/varadhi/verticles/webserver/WebServerVerticle.java(1 hunks)
💤 Files with no reviewable changes (1)
- server/src/main/java/com/flipkart/varadhi/config/AppConfiguration.java
🚧 Files skipped from review as they are similar to previous changes (14)
- entities/src/main/java/module-info.java
- entities/build.gradle
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NandCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/EndsWithCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ContainsCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NotCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/NorCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/OrCondition.java
- server/src/main/java/com/flipkart/varadhi/verticles/webserver/WebServerVerticle.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/StartsWithCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/InCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/logical/ExistsCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/conditions/comparison/AndCondition.java
- entities/src/main/java/com/flipkart/varadhi/entities/config/MessageHeaderConfiguration.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: call-workflow-docker-build / build
🔇 Additional comments (1)
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java (1)
138-147: LGTM! Good error case handling.The test properly validates that malformed conditions throw the expected exception.
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java
Outdated
Show resolved
Hide resolved
entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java (2)
13-32: Inconsistent validation annotations in ContainsCondition.The ContainsCondition enforces non-null values with
required = truein @JsonProperty, but unlike StartsWithCondition and InCondition, it doesn't use @NotNull annotations. Consider adding @NotNull to maintain consistency across all condition classes.public record ContainsCondition(String key, String value) implements Condition { @JsonCreator public ContainsCondition( - @JsonProperty (value = "key", required = true) String key, - @JsonProperty (value = "value", required = true) String value + @JsonProperty (value = "key", required = true) @NotNull String key, + @JsonProperty (value = "value", required = true) @NotNull String value ) { this.key = key; this.value = value; }
79-94: Add @NotNull annotation to ExistsCondition.For consistency with other conditions, add the @NotNull annotation to the key parameter.
public record ExistsCondition(String key) implements Condition { @JsonCreator - public ExistsCondition(@JsonProperty (value = "key", required = true) String key) { + public ExistsCondition(@JsonProperty (value = "key", required = true) @NotNull String key) { this.key = key; }entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java (5)
12-28: Add null check and validation in AndCondition.The AndCondition should validate that the values list is not empty and handle the case where it could be null despite the @NotNull annotation.
public record AndCondition (List<Condition> values) implements Condition { @JsonCreator public AndCondition(@JsonProperty(value = "values", required = true) List<Condition> values) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("Values cannot be null or empty for AndCondition"); + } this.values = values; } @Override public String toString() { + if (values.isEmpty()) { + return "true"; // Empty AND evaluates to true + } return values.stream().map(Condition::toString).collect(Collectors.joining(" and ")); }
30-46: Add validation for NandCondition values.Similar to AndCondition, NandCondition should validate its input values list.
public record NandCondition(List<Condition> values) implements Condition { @JsonCreator public NandCondition(@JsonProperty (value = "values", required = true) List<Condition> values) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("Values cannot be null or empty for NandCondition"); + } this.values = values; }
48-64: Add validation for OrCondition values.Similar to other boolean conditions, OrCondition should validate its input values list.
public record OrCondition(List<Condition> values) implements Condition { @JsonCreator public OrCondition(@JsonProperty (value = "values", required = true) List<Condition> values) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("Values cannot be null or empty for OrCondition"); + } this.values = values; } @Override public String toString() { + if (values.isEmpty()) { + return "false"; // Empty OR evaluates to false + } return values.stream().map(Condition::toString).collect(Collectors.joining(" or ")); }
66-82: Add validation for NorCondition values.Similar to other boolean conditions, NorCondition should validate its input values list.
public record NorCondition(List<Condition> values) implements Condition { @JsonCreator public NorCondition(@JsonProperty (value = "values", required = true) List<Condition> values) { + if (values == null || values.isEmpty()) { + throw new IllegalArgumentException("Values cannot be null or empty for NorCondition"); + } this.values = values; }
84-100: Add null check for NotCondition value.NotCondition should validate that its value is not null.
public record NotCondition(Condition value) implements Condition { @JsonCreator public NotCondition(@JsonProperty (value = "value", required = true) Condition value) { + if (value == null) { + throw new IllegalArgumentException("Value cannot be null for NotCondition"); + } this.value = value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/Condition.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java(1 hunks)entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- entities/src/test/java/filters/FilterDeserialzeAndEvaluatorTest.java
🧰 Additional context used
🪛 GitHub Actions: Build Docker Image
entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java
[error] 1-1: Execution failed for task ':entities:spotlessJavaCheck'. The following files had format violations. Run './gradlew :entities:spotlessApply' to fix these violations.
🪛 GitHub Actions: E2E
entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java
[error] 1-1: Spotless formatting check failed. The following files had format violations: src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java. Run './gradlew :entities:spotlessApply' to fix these violations.
🔇 Additional comments (1)
entities/src/main/java/com/flipkart/varadhi/entities/filters/Condition.java (1)
1-41: Well-designed filter condition interface with clear contract.The Condition interface provides a clean and extensible foundation for the filter framework. The use of Jackson annotations for polymorphic serialization/deserialization is appropriate for supporting various condition types in JSON format. The documentation clearly explains the purpose and contract of the interface.
entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java (1)
15-15:⚠️ Potential issueFix spacing in @JsonProperty annotations.
There's a space between
@JsonPropertyand the opening parenthesis in all annotations which is causing spotless formatting violations. Remove these spaces to fix the pipeline failures.- public AndCondition(@JsonProperty (value = "values", required = true) List<Condition> values) { + public AndCondition(@JsonProperty(value = "values", required = true) List<Condition> values) {Apply this fix to all JsonProperty annotations in the file (lines 15, 34, 53, 72, and 91).
Also applies to: 34-34, 53-53, 72-72, 91-91
🧹 Nitpick comments (7)
entities/src/test/java/filters/StringConditionsTest.java (1)
17-28: Suggestion: Add assertion messages for better test diagnostics.When assertions fail, it would be helpful to have descriptive messages to quickly identify which condition failed and why.
- assertTrue(condition.evaluate(headers)); + assertTrue(condition.evaluate(headers), "Contains condition should match when value is present"); - assertFalse(condition.evaluate(headers)); + assertFalse(condition.evaluate(headers), "Contains condition should not match when value is absent");server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (1)
378-391: LGTM! Test for header key size validation.This test ensures that the system properly validates header keys that exceed the allowed size and returns appropriate error responses.
Consider extracting the magic number 101 into a named constant for better readability:
+ private static final int OVERSIZED_HEADER_KEY_LENGTH = 101; @Test public void testProduceWithHighHeaderKeySize() throws InterruptedException { - String randomString = RandomString.make(101); + String randomString = RandomString.make(OVERSIZED_HEADER_KEY_LENGTH);entities/src/test/java/filters/BooleanConditionsTest.java (1)
1-22: Fix duplicate imports and unnecessary blank lines.There are duplicate imports for ArrayListMultimap and Multimap classes, and several unnecessary blank lines.
package filters; import java.util.List; import com.flipkart.varadhi.entities.filters.BooleanConditions; import com.flipkart.varadhi.entities.filters.Condition; import com.flipkart.varadhi.entities.filters.StringConditions; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Multimap; import org.junit.jupiter.api.BeforeAll; - - -import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.Multimap; import org.junit.jupiter.api.Test; -import java.util.List; - import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue;server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java (2)
20-92: LGTM! Comprehensive test data for different condition types.The
provideConditionsmethod provides a robust set of test cases covering various condition types and combinations.Consider breaking down the large test data provider into smaller, more focused methods based on condition type for better maintainability:
private static Stream<Arguments> provideAndConditions() { return Stream.of( Arguments.of( "[{\"op\":\"AND\",\"values\":[{\"op\":\"startsWith\",\"key\":\"X_abc\",\"value\":\"my_prefix\"},{\"op\":\"endsWith\",\"key\":\"X_abc\",\"value\":\"suffix\"}]}]", false, "startsWith(X_abc,\"my_prefix\") and endsWith(X_abc,\"suffix\")" ), // Additional AND conditions ); } private static Stream<Arguments> provideStringConditions() { return Stream.of( Arguments.of( "[{\"op\":\"startsWith\",\"key\":\"X_abc\",\"value\":\"my_prefix\"}]", true, "startsWith(X_abc,\"my_prefix\")" ), // Additional string conditions ); } // Combine them in provideConditions private static Stream<Arguments> provideConditions() { return Stream.concat( Stream.concat(provideAndConditions(), provideStringConditions()), // Additional condition streams ); }
123-144: Consider using behavior-based testing rather than class-based checks.The test relies on checking the specific class of conditions to determine expected behavior with empty headers. This approach is brittle if implementation details change.
for (Condition condition : conditions) { - if (condition.getClass() == BooleanConditions.NandCondition.class || condition.getClass() - == BooleanConditions.NorCondition.class - || condition.getClass() == BooleanConditions.NotCondition.class) { + // Identify negated conditions by their string representation + if (condition.toString().startsWith("not(")) { assertTrue(condition.evaluate(headers)); } else { assertFalse(condition.evaluate(headers)); } }entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java (2)
1-11: Add class-level JavaDoc documentation.This class lacks JavaDoc comments explaining its purpose and how the various condition types should be used. Adding documentation would improve code maintainability.
package com.flipkart.varadhi.entities.filters; import java.util.List; import java.util.stream.Collectors; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.collect.Multimap; +/** + * Provides boolean logic implementations of the Condition interface. + * These classes can be used to create complex boolean expressions for filtering. + */ public class BooleanConditions {
83-83: Consider alternate implementation for consistency.While
noneMatchis correct for NOR, an alternative implementation could be!values.stream().anyMatch(condition -> condition.evaluate(headers))for consistency with other negated conditions like NAND.Both implementations are logically equivalent, so this is just a style suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java(1 hunks)entities/src/test/java/filters/BooleanConditionsTest.java(1 hunks)entities/src/test/java/filters/StringConditionsTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/test/java/com/flipkart/varadhi/web/produce/PreProduceHandlerTest.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-workflow-docker-build / build
- GitHub Check: build
🔇 Additional comments (25)
entities/src/test/java/filters/StringConditionsTest.java (5)
1-14: LGTM! Good unit test structure for string conditions.The test class is well-organized and tests each condition type separately, which makes it easy to isolate issues if tests fail.
30-42: LGTM! Test for StartsWithCondition looks good.The test correctly validates that the condition passes when a string starts with the specified value and fails otherwise.
44-56: LGTM! Test for EndsWithCondition looks good.The test correctly validates that the condition passes when a string ends with the specified value and fails otherwise.
58-69: LGTM! Test for ExistsCondition looks good.The test correctly validates that the condition passes when a key exists and fails when it doesn't.
71-83: LGTM! Test for InCondition looks good.The test correctly validates that the condition passes when a key's value is in the specified list and fails otherwise.
server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (5)
36-36: LGTM! Adding RandomString import for test data generation.The import is needed for the new test methods that check high header key and body sizes.
52-52: LGTM! Defined maximum request size constant.Good practice to define the maximum request size as a constant for better readability and maintainability.
71-72: LGTM! Added mock return value for produceToTopic.This ensures tests can run without actual service calls, making them more isolated and predictable.
77-80: Add HeaderUtils.deInitialize() in PostTest to clean up global state.HeaderUtils is initialized in the setup method but not deinitialized in teardown. This could potentially affect other tests if they rely on different header configurations.
@AfterEach public void PostTest() throws InterruptedException { + HeaderUtils.deInitialize(); tearDown(); }
393-411: LGTM! Test for request body size validation.This test ensures that the system properly validates request bodies that exceed the maximum allowed size and returns appropriate error responses.
entities/src/test/java/filters/BooleanConditionsTest.java (6)
23-31: LGTM! Good setup for test conditions.Using static fields and a BeforeAll method is efficient for sharing test fixtures across multiple test methods.
33-48: LGTM! Test for AndCondition looks good.The test correctly verifies that AndCondition evaluates to true only when all conditions are satisfied.
50-65: LGTM! Test for NandCondition looks good.The test correctly verifies that NandCondition evaluates to false when all conditions are satisfied and true otherwise.
67-83: LGTM! Test for OrCondition looks good.The test correctly verifies that OrCondition evaluates to true when at least one condition is satisfied.
85-103: LGTM! Test for NorCondition looks good.The test correctly verifies that NorCondition evaluates to false when at least one condition is satisfied and true otherwise.
105-117: LGTM! Test for NotCondition looks good.The test correctly verifies that NotCondition evaluates to the opposite of the condition it wraps.
server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java (5)
1-19: LGTM! Good import organization.Imports are well-organized and contain all the necessary classes for testing filter serialization and deserialization.
94-102: LGTM! Good test cases for malformed conditions.Testing error cases is important to ensure the system handles invalid input gracefully.
104-121: LGTM! Test for condition evaluation with populated headers.The test correctly deserializes JSON conditions and evaluates them against the provided headers.
146-159: LGTM! Good test for condition string representation.The test verifies that the string representation of conditions matches the expected output.
161-170: LGTM! Test for malformed JSON handling.The test correctly verifies that the system throws appropriate exceptions when parsing malformed JSON.
entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java (4)
12-28: Consider handling empty conditions list.The
evaluatemethod returns true for an empty values list due toallMatchreturning true for empty collections. It's unclear if this is the intended behavior.Consider adding validation to ensure the list is not empty or explicitly document that an empty AND condition always evaluates to true.
31-47: Consider handling empty conditions list for NandCondition.The
evaluatemethod returns false for an empty values list due to the negation ofallMatch. Verify if this is the intended behavior.Consider adding validation or documenting that an empty NAND condition always evaluates to false.
50-66: Consider handling empty conditions list for OrCondition.The
evaluatemethod returns false for an empty values list due toanyMatchreturning false for empty collections. Verify if this is the intended behavior.Consider adding validation or documenting that an empty OR condition always evaluates to false.
69-85: Consider handling empty conditions list for NorCondition.The
evaluatemethod returns true for an empty values list due tononeMatchreturning true for empty collections. Verify if this is the intended behavior.Consider adding validation or documenting that an empty NOR condition always evaluates to true.
Test Results 67 files 67 suites 1m 0s ⏱️ Results for commit 4bc5626. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
conf/configuration.yml (1)
103-103: YAML Newline Warning: Missing New Line at End of File
YAMLlint has pointed out that there is no trailing newline at the end of the file. Please add a newline character at the end of this file to adhere to standard formatting guidelines and pass the YAML lint check.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 103-103: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
conf/configuration.yml(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java(1 hunks)entities/src/test/java/filters/BooleanConditionsTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- entities/src/test/java/filters/BooleanConditionsTest.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/StringConditions.java
- entities/src/main/java/com/flipkart/varadhi/entities/filters/BooleanConditions.java
🧰 Additional context used
🪛 YAMLlint (1.35.1)
conf/configuration.yml
[error] 103-103: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-workflow-docker-build / build
- GitHub Check: build
🔇 Additional comments (7)
conf/configuration.yml (1)
102-103: Formatting Improvement: Removal of Blank Line
The removal of the extra blank line before thefilterNonCompliantHeadersentry enhances the consistency and readability of the YAML structure. This change is non-functional and aligns with the broader formatting updates across the repository.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 103-103: no new line character at the end of file
(new-line-at-end-of-file)
server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (6)
36-36: Appropriate addition of RandomString import.The RandomString utility is correctly imported and used in the new test methods for generating random strings of specific lengths.
52-53: Constants for size limits well defined.These constants clearly define the maximum request size (5MB) and the maximum header key length (101 characters), making the test cases more maintainable and the size limits explicit.
72-73: Mock setup for ProduceResult in PreTest.Good addition of the mock return value for the
produceToTopicmethod in the setup, which ensures that tests can run consistently without actual service calls.
78-81: Add HeaderUtils.deInitialize() in PostTest to clean up global state.HeaderUtils is initialized in the setup method but not deinitialized in teardown. This could potentially affect other tests if they rely on different header configurations.
@AfterEach public void PostTest() throws InterruptedException { + HeaderUtils.deInitialize(); tearDown(); }
379-392: LGTM: Well-structured test for header size limit.This test case properly validates that the application correctly handles header keys that exceed the allowed size limit of 100 characters, returning a 400 error with an appropriate error message.
394-412: LGTM: Comprehensive test for request body size limit.This test effectively verifies that the application enforces the maximum request size limit (5MB) by:
- Creating a request body larger than the limit
- Checking that a 400 error is returned with the correct error message
The test is well-structured with clear comments explaining the approach.
server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java
Show resolved
Hide resolved
server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java (3)
20-26: Setup looks good, but consider making mapper finalThe setup of the ObjectMapper looks correct using JsonMapper utility. Since the mapper is only set once in the @BeforeAll method and never changed, consider making it final:
- private static ObjectMapper mapper; + private static final ObjectMapper mapper = JsonMapper.getMapper();Then you could remove the setUp() method entirely or keep it for any additional setup that might be needed in the future.
36-111: Test data covers a good range of filter conditionsThe test data generation looks comprehensive, covering various boolean operations (AND, OR, NAND, NOR, NOT) and string conditions (startsWith, endsWith, contains, exists, in). However, there are a few points to improve:
- The logic using
emptyHeaderand!emptyHeaderfor expected results is not immediately obvious and would benefit from a comment explaining the relationship.- Consider adding a brief comment for each test case explaining what it's testing.
159-168: Reuse the static mapper instanceThe
testMalformedConditionsmethod creates a new instance of ObjectMapper, but there's already a static one available as a class member.-ObjectMapper mapper = JsonMapper.getMapper(); +// Using the class-level mapper instanceThis is a small optimization but maintains consistency in the test class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-workflow-docker-build / build
- GitHub Check: build
🔇 Additional comments (2)
server/src/test/java/com/flipkart/varadhi/web/FilterSerializerAndDeserializerTest.java (2)
123-140: Parameter names could be more descriptiveThe current parameter names aren't very self-explanatory. Consider renaming them to better reflect their purpose:
-public void testConditions(String json, boolean expected, String result) throws JsonProcessingException { +public void testConditions(String jsonInput, boolean expectedEvaluation, String expectedStringRepresentation) throws JsonProcessingException {And update the assertion lines accordingly.
This was previously raised in a comment where a reviewer mentioned "you are not using expected and result param in the test." I see that they are actually being used in the assertions, but more descriptive names would clarify their purpose.
113-121: Good test coverage for error casesThe test cases for malformed conditions appropriately test various error scenarios like unknown operations, missing values, etc. This helps ensure the deserializer handles errors correctly.
| @ParameterizedTest | ||
| @MethodSource ("provideConditionsWithEmptyHeader") | ||
| public void testConditionsWithEmptyHeader(String json, boolean expected, String result) | ||
| throws JsonProcessingException { | ||
| List<Condition> conditions = mapper.readValue( | ||
| json, | ||
| mapper.getTypeFactory().constructCollectionType(List.class, Condition.class) | ||
| ); | ||
|
|
||
| Multimap<String, String> headers = ArrayListMultimap.create(); | ||
|
|
||
| for (Condition condition : conditions) { | ||
| assertEquals(expected, condition.evaluate(headers)); | ||
| assertEquals(result, condition.toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate deserialization code between test methods
The code for deserializing the JSON conditions is duplicated between testConditions and testConditionsWithEmptyHeader. Consider extracting this common logic to a helper method:
+private List<Condition> deserializeConditions(String json) throws JsonProcessingException {
+ return mapper.readValue(
+ json,
+ mapper.getTypeFactory().constructCollectionType(List.class, Condition.class)
+ );
+}Then call this method from both test methods.
📝 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.
| @ParameterizedTest | |
| @MethodSource ("provideConditionsWithEmptyHeader") | |
| public void testConditionsWithEmptyHeader(String json, boolean expected, String result) | |
| throws JsonProcessingException { | |
| List<Condition> conditions = mapper.readValue( | |
| json, | |
| mapper.getTypeFactory().constructCollectionType(List.class, Condition.class) | |
| ); | |
| Multimap<String, String> headers = ArrayListMultimap.create(); | |
| for (Condition condition : conditions) { | |
| assertEquals(expected, condition.evaluate(headers)); | |
| assertEquals(result, condition.toString()); | |
| } | |
| } | |
| @ParameterizedTest | |
| @MethodSource("provideConditionsWithEmptyHeader") | |
| public void testConditionsWithEmptyHeader(String json, boolean expected, String result) | |
| throws JsonProcessingException { | |
| - List<Condition> conditions = mapper.readValue( | |
| - json, | |
| - mapper.getTypeFactory().constructCollectionType(List.class, Condition.class) | |
| - ); | |
| + List<Condition> conditions = deserializeConditions(json); | |
| Multimap<String, String> headers = ArrayListMultimap.create(); | |
| for (Condition condition : conditions) { | |
| assertEquals(expected, condition.evaluate(headers)); | |
| assertEquals(result, condition.toString()); | |
| } | |
| } | |
| + | |
| + private List<Condition> deserializeConditions(String json) throws JsonProcessingException { | |
| + return mapper.readValue( | |
| + json, | |
| + mapper.getTypeFactory().constructCollectionType(List.class, Condition.class) | |
| + ); | |
| + } |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style