Add backwards-compatible serialization for filtration #840
Conversation
### New Additions * **`FilterableAttribute`** is a flattened POJO implementation of the new [granular filtration feature](meilisearch/meilisearch#5163). It contains backwards compatibility with legacy data. * **`GsonFilterableAttributeSerializer`** allows for the serialization/deserialization of these POJOs from legacy primitive string filter attributes, as well as the persistence of new single-string attributes with default settings in legacy format. ### Changes * **`GsonJsonHandler`** registers our new serializer. * **`Index`**: `getFilterableAttributesSettings` is updated to accommodate the new POJO. Backwards compatible `legacyGetFilterableAttributesSettings` which returns the traditional `String[]` attribute configuration has been added as well. `updateFilterableAttributes` has been updated to accept `Object[]`, as polymorphic backwards compatibility which can accept `null` values, requires no ambiguity; as we cannot do a polymorphic method, this seemed the best fit. * **`Settings`**: `filterableAttributes` was updated to now be of type `FilterableAttribute[]`. * **`SettingsHandler`**: `updateFilterableAttributesSettings` now accepts a `FilterableAttribute[]` object. `getFilterableAttributesSettings` now returns one likewise.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new FilterableAttribute POJO along with a backwards-compatible Gson serializer/deserializer and updates to Settings and Index to support the new filtration format while preserving legacy functionality. Key changes include:
- Adding new serialization/deserialization logic for FilterableAttribute in GsonFilterableAttributeSerializer and its tests.
- Updating Settings and Index classes to handle FilterableAttribute arrays and provide legacy support.
- Modifying integration tests and code samples to work with the new filtration attributes.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java | Unit tests for verifying legacy and new serialization behaviors. |
| src/test/java/com/meilisearch/integration/TasksTest.java | Integration tests now print debug information for task queries. |
| src/test/java/com/meilisearch/integration/SettingsTest.java | Tests updated to use legacy methods and new methods for filterable attributes settings. |
| src/test/java/com/meilisearch/integration/FilterableAttributeTest.java | Unit tests for the FilterableAttribute POJO constructors and validations. |
| src/main/java/com/meilisearch/sdk/model/Settings.java | Updated settings to use FilterableAttribute[] and added a helper to convert legacy strings. |
| src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java | New POJO implementation for filtration settings with validation for geo patterns. |
| src/main/java/com/meilisearch/sdk/json/GsonJsonHandler.java | Registers the new serializer for FilterableAttribute. |
| src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializer.java | Implements Gson serialization and deserialization for FilterableAttribute. |
| src/main/java/com/meilisearch/sdk/SettingsHandler.java | Modified to support FilterableAttribute in get/update/reset settings. |
| src/main/java/com/meilisearch/sdk/Index.java | Updated to provide both legacy and new methods for filterable attributes settings. |
| .code-samples.meilisearch.yaml | Updated samples to demonstrate the new filterable attributes setup. |
Comments suppressed due to low confidence (3)
.code-samples.meilisearch.yaml:233
- There is an inconsistency with class and variable names – use 'FilterableAttribute' instead of 'FilterableAttributes' and ensure that the correct variable (e.g., 'filtersTypes') is passed in the constructor.
settings.setFilterableAttributes(new FilterableAttributes[] {new FilterableAttributes("genres"), new FilterableAttributes(new String[]{"director"}, true, filters)});
src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java:98
- The testDoubleEncoding: 'array' already holds the encoded string value, so re-encoding it may lead to incorrect output. Compare 'array' directly with expectedOutput to ensure correct behavior.
assertEquals(expectedOutput, handler.encode(array));
src/test/java/com/meilisearch/integration/TasksTest.java:145
- [nitpick] Remove the debugging print statements; they are likely leftover from development and are not needed in committed integration tests.
System.out.println("Expected from: " + from);
Co-authored-by: Laurent Cazanove <[email protected]>
|
Hello! Thank you for your PR @Noah-Mack-01! ❤️ If I understand correctly, this PR introduces breaking changes, because people need to use the Also, is there a way to avoid breaking change? Make the new methods to accept both of format (array of string and array of object)? I would love having the review of @brunoocasali before merging that |
|
Hi @curquiza !! Thinking through this now. Its unfortunate we're working off such an old JDK, since Java 15's All update methods are generic, such as the following: However I can't think of a way to guarantee that the I suppose the answer would be to maintain the legacy implementation's original nomenclature |
Priority was placed on full backwards compatibility. Since two methods in the same java class cannot have the same name and input parameters (the compiler does not consider output class) the legacy feature will retain its same `getFilterableAttributesSettings` name and the new, full config getter will instead be called `getFullFilterableAttributesSettings[]`.
…erable-Attributes-Methods' into enhancement/meilisearch#838-Filterable-Attributes-Methods
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce support for the new, structured Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Index
participant SettingsHandler
participant GsonJsonHandler
User->>Index: updateFilterableAttributesSettings(attributes)
Index->>Index: Determine attribute type (String[] or FilterableAttribute[])
Index->>SettingsHandler: updateFilterableAttributesSettings(uid, FilterableAttribute[])
SettingsHandler->>GsonJsonHandler: Serialize FilterableAttribute[]
GsonJsonHandler-->>SettingsHandler: JSON payload
SettingsHandler->>Meilisearch API: PUT /settings/filterable-attributes
Meilisearch API-->>SettingsHandler: TaskInfo
SettingsHandler-->>Index: TaskInfo
Index-->>User: TaskInfo
User->>Index: getFullFilterableAttributesSettings()
Index->>SettingsHandler: getFilterableAttributesSettings(uid)
SettingsHandler->>Meilisearch API: GET /settings/filterable-attributes
Meilisearch API-->>SettingsHandler: JSON response
SettingsHandler->>GsonJsonHandler: Deserialize JSON to FilterableAttribute[]
GsonJsonHandler-->>SettingsHandler: FilterableAttribute[]
SettingsHandler-->>Index: FilterableAttribute[]
Index-->>User: FilterableAttribute[]
Assessment against linked issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🔭 Outside diff range comments (1)
src/test/java/com/meilisearch/integration/SettingsTest.java (1)
774-776:⚠️ Potential issueWrong reset method called – test is exercising the wrong API
In the sortable attributes test you reset filterable attributes
instead of sortable ones, so the assertions are not validating what
the test name describes.- index.waitForTask(index.resetFilterableAttributesSettings().getTaskUid()); - String[] filterableAttributesAfterReset = index.getFilterableAttributesSettings(); + index.waitForTask(index.resetSortableAttributesSettings().getTaskUid()); + String[] sortableAttributesAfterReset = index.getSortableAttributesSettings();Update the variable names accordingly.
🧹 Nitpick comments (7)
src/test/java/com/meilisearch/integration/TasksTest.java (1)
145-147: Consider removing debugging print statements.These print statements appear to be added for debugging purposes during development and don't provide value in automated test execution. It's generally best practice to remove debug outputs before submitting a PR, as they can create noise in test logs.
- System.out.println("Expected from: " + from); - System.out.println("Actual from: " + result.getFrom()); - System.out.println("Tasks returned: " + Arrays.toString(result.getResults()));.code-samples.meilisearch.yaml (1)
230-233: Updates example to demonstrate structured filterable attributesThe example now shows how to use the new FilterableAttribute class with filter types.
However, there's a small issue in the code:
- settings.setFilterableAttributes(new FilterableAttribute[] {new FilterableAttribute("genres"), new FilterableAttribute(new String[]{"director"}, true, filters)}); + settings.setFilterableAttributes(new FilterableAttribute[] {new FilterableAttribute("genres"), new FilterableAttribute(new String[]{"director"}, true, filtersTypes)});The variable is defined as
filtersTypeson line 231 but used asfilterson line 233.src/main/java/com/meilisearch/sdk/Index.java (1)
750-765: Inefficient & duplicate handler call – simplify stream processing
getFilterableAttributesSettings()invokessettingsHandler.getFilterableAttributesSettingstwice and uses an over-complicatedreduceto flatten patterns. The extra call wastes an HTTP round-trip, and thereduceallocates a new list at every accumulation step.A leaner, single-pass approach is both clearer and faster:
-FilterableAttribute[] attributes = - this.settingsHandler.getFilterableAttributesSettings(this.uid); -return Arrays.stream(this.settingsHandler.getFilterableAttributesSettings(this.uid)) - .reduce( - new ArrayList<String>(), - (list, next) -> { - list.addAll(Arrays.asList(next.getPatterns())); - return list; - }, - (a, b) -> { - a.addAll(b); - return a; - }) - .toArray(new String[0]); +FilterableAttribute[] attributes = + this.settingsHandler.getFilterableAttributesSettings(this.uid); +return Arrays.stream(attributes) + .flatMap(attr -> Arrays.stream(attr.getPatterns())) + .toArray(String[]::new);src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java (1)
59-70: Error aggregation prints “null” entries – improve message building
Arrays.toString(errors)will include the string"null"for slots
without an error, resulting in confusing messages such as
[null, Equality filter cannot be null..., null].A cleaner JDK-8-compatible build:
-String[] errors = new String[3]; -... -for (String error : errors) - if (error != null) - throw new RuntimeException( - "Invalid filter for geo pattern: " + Arrays.toString(errors)); +List<String> errors = new ArrayList<>(); +... +if (!errors.isEmpty()) { + throw new RuntimeException( + "Invalid filter for geo pattern: " + String.join("; ", errors)); +}src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java (2)
15-17: Remove unused field to avoid confusionThe
serializerinstance is declared but never assigned/used.
Keeping dead fields dilutes readability and can mislead maintainers about the test’s intent.- GsonFilterableAttributeSerializer serializer; - GsonJsonHandler handler; + GsonJsonHandler handler;
152-164: Potential NPE and asymmetric filter comparison
assertDeserializedOutputsEqualsiterates only overa.getFilter().keySet().
If either side contains additional keys, orgetFilter()returnsnull, the method can throw or silently miss mismatches.- for (String key : a.getFilter().keySet()) { - assertEquals(a.getFilter().get(key), b.getFilter().get(key)); - } + Map<String, Boolean> filtersA = Optional.ofNullable(a.getFilter()).orElse(Map.of()); + Map<String, Boolean> filtersB = Optional.ofNullable(b.getFilter()).orElse(Map.of()); + + assertEquals(filtersA.size(), filtersB.size()); + for (Map.Entry<String, Boolean> entry : filtersA.entrySet()) { + assertEquals(entry.getValue(), filtersB.get(entry.getKey())); + }src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializer.java (1)
68-70: Exception message building hides root causesUsing
String.join("\n", Arrays.toString(exceptions.toArray()))collapses the list into a single string (the delimiter is ignored) and prints theException.toString()rather than the message.- throw new JsonParseException(String.join("\n", Arrays.toString(exceptions.toArray()))); + String message = exceptions.stream() + .map(Throwable::getMessage) + .collect(Collectors.joining("\n")); + throw new JsonParseException(message);Remember to
import java.util.stream.Collectors;.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.code-samples.meilisearch.yaml(1 hunks)src/main/java/com/meilisearch/sdk/Index.java(3 hunks)src/main/java/com/meilisearch/sdk/SettingsHandler.java(3 hunks)src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializer.java(1 hunks)src/main/java/com/meilisearch/sdk/json/GsonJsonHandler.java(2 hunks)src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java(1 hunks)src/main/java/com/meilisearch/sdk/model/Settings.java(2 hunks)src/test/java/com/meilisearch/integration/FilterableAttributeTest.java(1 hunks)src/test/java/com/meilisearch/integration/SettingsTest.java(2 hunks)src/test/java/com/meilisearch/integration/TasksTest.java(1 hunks)src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java (1)
src/main/java/com/meilisearch/sdk/model/Settings.java (1)
Getter(13-44)
🔇 Additional comments (9)
src/main/java/com/meilisearch/sdk/json/GsonJsonHandler.java (2)
10-10: Adds required import for new classThis import is necessary for the registration of the FilterableAttribute class with Gson.
20-21: Adds custom type adapter for the new FilterableAttribute classThe registration of GsonFilterableAttributeSerializer enables the JSON handler to support both legacy string format and the new object format for filterable attributes.
src/main/java/com/meilisearch/sdk/model/Settings.java (2)
20-20: Updates field type to support new structured filterable attributesChanging from
String[]toFilterableAttribute[]enables storing richer metadata associated with filterable attributes.
38-43: Adds backward-compatible setter methodThis method provides backward compatibility by converting simple strings to FilterableAttribute objects, allowing existing code to continue working without changes.
src/main/java/com/meilisearch/sdk/SettingsHandler.java (2)
5-5: Optimizes imports using wildcardReplaced individual model class imports with a wildcard import, which is cleaner and easier to maintain as new model classes are added.
316-320:✅ Verification successful
Updates method return type for filterable attributes
Changed the return type from
String[]toFilterableAttribute[]to support the new structured format. This is a breaking change at this level, but according to the PR description, backward compatibility is maintained at the Index class level with legacy methods.Let's verify the backward compatibility handling in the Index class:
🏁 Script executed:
#!/bin/bash # Find the backward-compatible methods in Index class rg "legacyGetFilterableAttributesSettings" --type javaLength of output: 54
To confirm how backward compatibility is exposed, let’s locate the
Indexclass and search for any methods handling “filterable” attributes:
🏁 Script executed:
#!/bin/bash # 1. Find the Index class file fd Index.java # 2. Search for any filterable-related methods across Java sources rg -n "filterable" --type javaLength of output: 7009
Let’s inspect how
getFilterableAttributesSettingsis implemented inIndex.javato confirm backward compatibility:
🏁 Script executed:
#!/bin/bash # Show the getFilterableAttributesSettings methods with context rg -n "getFilterableAttributesSettings" -C3 src/main/java/com/meilisearch/sdk/Index.javaLength of output: 1602
Backward compatibility for filterable attributes methods confirmed
- SettingsHandler.getFilterableAttributesSettings now returns FilterableAttribute[]
- Index#getFullFilterableAttributesSettings() exposes the new FilterableAttribute[] return type
- Index#getFilterableAttributesSettings() remains as String[] by converting the structured response
No further changes needed.
src/test/java/com/meilisearch/integration/SettingsTest.java (1)
710-715: Logical mismatch: assertions after reset still use new patternsAfter a successful reset we expect the attributes to match the initial
state, not the interim new list. Re-checking against
newFilterableAttributesmay lead to false positives (loops simply
don’t execute when the array is empty).Please verify that the post-reset values equal
initialFilterableAttributesinstead.src/test/java/com/meilisearch/integration/FilterableAttributeTest.java (1)
12-19: Good coverage of default constructor behaviourThe test accurately checks patterns, facetSearch and default filters for
a single-pattern attribute. Nice work!src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializer.java (1)
137-140:JsonArray#asList()is available only from Gson 2.10Projects pinned to older Gson versions (quite common in Android stacks) will not compile.
Consider a version-agnostic fallback:JsonArray array = object.getAsJsonArray("attributePatterns"); patterns = StreamSupport.stream(array.spliterator(), false) .map(JsonElement::getAsString) .toArray(String[]::new);
| TaskInfo updateFilterableAttributesSettings( | ||
| String uid, FilterableAttribute[] filterableAttributes) throws MeilisearchException { | ||
| return httpClient.put( | ||
| settingsPath(uid).addSubroute("filterable-attributes").getURL(), | ||
| filterableAttributes == null | ||
| ? httpClient.jsonHandler.encode(filterableAttributes) | ||
| : filterableAttributes, | ||
| httpClient.jsonHandler.encode(filterableAttributes), | ||
| TaskInfo.class); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Updates update method parameter type and simplifies encoding
Changed the parameter type from String[] to FilterableAttribute[] and simplified the JSON encoding logic. The removal of the null check is safe because encode() already handles null values.
Let's verify how the Update method is called:
🏁 Script executed:
#!/bin/bash
# Find calls to updateFilterableAttributesSettings
rg "updateFilterableAttributesSettings" --type javaLength of output: 2055
Breaking Change: UpdateFilterableAttributesSettings Signature Now Requires FilterableAttribute[]
The parameter type was changed from String[] to FilterableAttribute[] and the null‐check was removed (since encode() already handles null). This is a breaking change: all existing call sites passing String[] or relying on the generic overload will fail to compile.
Please update the following locations:
-
src/main/java/com/meilisearch/sdk/Index.java
The generic methodpublic <O> TaskInfo updateFilterableAttributesSettings(O[] filterableAttributes)
now passes
O[]directly toSettingsHandler#updateFilterableAttributesSettings(String, FilterableAttribute[]). You must either:- Add an overload taking
String... filterableAttributesthat converts eachStringto aFilterableAttribute; or - Change all call sites to use
FilterableAttributeenums directly.
- Add an overload taking
-
src/test/java/com/meilisearch/integration/SettingsTest.java
-
src/test/java/com/meilisearch/integration/DocumentsTest.java
Update calls likeindex.updateFilterableAttributesSettings(new String[] {"genres"}); index.updateFilterableAttributesSettings(null);
to use
FilterableAttribute[](or your newString...overload).
No change needed in SettingsHandler.java beyond the already applied signature update.
src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java
Outdated
Show resolved
Hide resolved
| "[{\"attributePatterns\":[\"attribute1\",\"attribute2\"],\"features\":{\"facetSearch\":true,\"filter\":{\"comparison\":true,\"equality\":false}}},\"attribute3\"]"; | ||
| String array = handler.encode(new FilterableAttribute[] {input, input2}); | ||
| assertEquals(expectedOutput, handler.encode(array)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deterministic JSON comparison
Even after the above fix, the test performs a string comparison.
HashMap does not guarantee key order, so the produced JSON property order can vary between JDKs, causing nondeterministic failures.
Prefer structural comparison:
JsonElement expected = JsonParser.parseString(expectedOutput);
JsonElement actual = JsonParser.parseString(result);
assertEquals(expected, actual);
src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializer.java
Outdated
Show resolved
Hide resolved
…Serializer.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…SerializerTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java (5)
61-61: Fix invalid JSON format in test inputThe test input contains unquoted property names (
attributePatterns), which is not valid JSON syntax. Even if the parser is lenient, using invalid JSON in tests makes them harder to understand and potentially brittle.- String input = "[{attributePatterns:[\"attribute1\", \"attribute2\"]},\"attribute3\"]"; + String input = "[{\"attributePatterns\":[\"attribute1\", \"attribute2\"]},\"attribute3\"]";
77-77: Fix invalid JSON format in test inputThe JSON string contains unquoted property names (
attributePatterns,features,facetSearch, etc.), which is not valid JSON. Proper JSON requires double quotes around property names.- String input = - "{attributePatterns:[\"attribute1\", \"attribute2\"],features: {facetSearch:true, filter:{equality:true, comparison:true}}}"; + String input = + "{\"attributePatterns\":[\"attribute1\", \"attribute2\"],\"features\": {\"facetSearch\":true, \"filter\":{\"equality\":true, \"comparison\":true}}}";
97-98: Fix double JSON encoding issue in assertionThe test currently encodes the array twice, which will cause the test to fail. The
handler.encode(array)call encodes a string that's already encoded.- String array = handler.encode(new FilterableAttribute[] {input, input2}); - assertEquals(expectedOutput, handler.encode(array)); + String result = handler.encode(new FilterableAttribute[] {input, input2}); + assertEquals(expectedOutput, result);
109-109: Fix invalid JSON format in test inputThe test input contains unquoted property names, which is not valid JSON.
- String input = - "{attributePatterns:[\"attribute1\"],features: {facetSearch:true, filter:{equality:false, comparison:false}}}"; + String input = + "{\"attributePatterns\":[\"attribute1\"],\"features\": {\"facetSearch\":true, \"filter\":{\"equality\":false, \"comparison\":false}}}";
121-121: Fix invalid JSON format in test inputThe test input contains unquoted property names, which is not valid JSON.
- String input = "{features: {facetSearch:true, filter:{equality:true, comparison:true}}}"; + String input = "{\"features\": {\"facetSearch\":true, \"filter\":{\"equality\":true, \"comparison\":true}}}";
🧹 Nitpick comments (3)
src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java (3)
152-165: Enhance the filter map comparison in assertDeserializedOutputsEqualsThe current implementation only checks that keys in
a.getFilter()have matching values inb.getFilter(), but doesn't verify that both maps have the same set of keys. This could miss cases wherebhas extra keys not present ina.private static void assertDeserializedOutputsEquals( FilterableAttribute a, FilterableAttribute b) { if (a == null) assertNull(b); else { assertEquals(a.getPatterns().length, b.getPatterns().length); for (int i = 0; i < a.getPatterns().length; i++) { assertEquals(a.getPatterns()[i], b.getPatterns()[i]); } assertEquals(a.getFacetSearch(), b.getFacetSearch()); + assertEquals(a.getFilter().size(), b.getFilter().size(), "Filter maps should have same number of keys"); for (String key : a.getFilter().keySet()) { assertEquals(a.getFilter().get(key), b.getFilter().get(key)); } } }
96-96: Use a deterministic JSON comparison approachString comparison of JSON can fail due to property order differences, since HashMap doesn't guarantee key order. Consider using a structural comparison instead.
- String expectedOutput = - "[{\"attributePatterns\":[\"attribute1\",\"attribute2\"],\"features\":{\"facetSearch\":true,\"filter\":{\"comparison\":true,\"equality\":false}}},\"attribute3\"]"; - String array = handler.encode(new FilterableAttribute[] {input, input2}); - assertEquals(expectedOutput, handler.encode(array)); + String result = handler.encode(new FilterableAttribute[] {input, input2}); + + // Parse to JSON elements for structural comparison + com.google.gson.JsonElement expected = com.google.gson.JsonParser.parseString(expectedOutput); + com.google.gson.JsonElement actual = com.google.gson.JsonParser.parseString(result); + assertEquals(expected, actual);
23-26: Empty @AfterEach method can be removedThe
postTestCleanupmethod is empty with just a comment. Since it's not performing any actual cleanup, it can be safely removed to reduce code clutter.- @AfterEach - public void postTestCleanup() { - // Cleanup after each test, if needed - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/test/java/com/meilisearch/sdk/json/GsonFilterableAttributeSerializerTest.java(1 hunks)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java (6)
60-72: Improve error validation mechanism for more efficient reportingThe current implementation accumulates all possible errors in an array and throws a single exception containing all errors. However, this approach creates the errors array even when validation passes. Consider a more efficient approach that reports the first error encountered.
- String[] errors = new String[3]; - if (!filters.containsKey("comparison") || filters.get("comparison") == false) - errors[0] = "Comparison filter cannot be null for '_geo' pattern"; - // rewrite the below lines to be JDK 8 compatible. - if (!filters.containsKey("equality") || filters.get("equality") == false) - errors[1] = "Equality filter cannot be null for '_geo' pattern"; - if (!facetSearch) errors[2] = "Facet search cannot be null for '_geo' pattern"; - for (String error : errors) - if (error != null) - throw new RuntimeException( - "Invalid filter for geo pattern: " + Arrays.toString(errors)); + if (!filters.containsKey("comparison") || !filters.get("comparison")) { + throw new RuntimeException("Invalid filter for geo pattern: Comparison filter must be true for '_geo' pattern"); + } + if (!filters.containsKey("equality") || !filters.get("equality")) { + throw new RuntimeException("Invalid filter for geo pattern: Equality filter must be true for '_geo' pattern"); + } + if (!facetSearch) { + throw new RuntimeException("Invalid filter for geo pattern: Facet search must be true for '_geo' pattern"); + }
64-64: Remove unnecessary commentThe comment "rewrite the below lines to be JDK 8 compatible" appears to be obsolete, as the code is already JDK 8 compatible.
- // rewrite the below lines to be JDK 8 compatible.
62-66: Simplify boolean conditionsThe conditions
filters.get("comparison") == falseandfilters.get("equality") == falsecan be simplified using the!operator for better readability.- if (!filters.containsKey("comparison") || filters.get("comparison") == false) + if (!filters.containsKey("comparison") || !filters.get("comparison")) errors[0] = "Comparison filter cannot be null for '_geo' pattern"; // rewrite the below lines to be JDK 8 compatible. - if (!filters.containsKey("equality") || filters.get("equality") == false) + if (!filters.containsKey("equality") || !filters.get("equality")) errors[1] = "Equality filter cannot be null for '_geo' pattern";
20-27: Consider creating defensive copies in the first constructorFor consistency with the third constructor, consider creating defensive copies of the filter map to prevent external modification after object creation.
public FilterableAttribute(String pattern) { boolean patternIsGeo = _GEO.equals(pattern); this.patterns = new String[] {pattern}; this.facetSearch = patternIsGeo; - this.filter = new HashMap<>(); + Map<String, Boolean> tempFilter = new HashMap<>(); + tempFilter.put("equality", true); + tempFilter.put("comparison", patternIsGeo); + this.filter = Collections.unmodifiableMap(tempFilter); - this.filter.put("equality", true); - this.filter.put("comparison", patternIsGeo); }Since you're creating a new HashMap in this constructor, making it unmodifiable would ensure consistent immutability across all constructors and prevent clients from modifying the internal state.
37-41: Consider making the filter map unmodifiableFor consistency with the suggestion for the first constructor, consider making the filter map unmodifiable in the second constructor as well to ensure immutability.
this.facetSearch = patternHasGeo; - this.filter = new HashMap<>(); - this.filter.put("equality", true); - this.filter.put("comparison", patternHasGeo); + Map<String, Boolean> tempFilter = new HashMap<>(); + tempFilter.put("equality", true); + tempFilter.put("comparison", patternHasGeo); + this.filter = Collections.unmodifiableMap(tempFilter);
57-57: Consider making the filter map unmodifiableFor enhanced immutability, consider wrapping the filter map with
Collections.unmodifiableMap()to prevent modification after construction.- this.filter = new HashMap<>(filters); // defensive copy + this.filter = Collections.unmodifiableMap(new HashMap<>(filters)); // defensive copy + immutability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java (1)
src/main/java/com/meilisearch/sdk/model/Settings.java (1)
Getter(13-44)
🔇 Additional comments (1)
src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java (1)
44-58: Good implementation of defensive copiesThis constructor properly validates input parameters and creates defensive copies of the input arrays and maps, which prevents external modification and ensures encapsulation. This addresses the previous review comment about potential NPE and defensive-copy omissions.
| public FilterableAttribute(String[] patterns) { | ||
| // Special case of '_geo' pattern will apply special conditions to default attributes | ||
| boolean patternHasGeo = false; | ||
| for (String s : patterns) | ||
| if (_GEO.equals(s)) { | ||
| patternHasGeo = true; | ||
| break; | ||
| } | ||
| this.facetSearch = patternHasGeo; | ||
| this.filter = new HashMap<>(); | ||
| this.filter.put("equality", true); | ||
| this.filter.put("comparison", patternHasGeo); | ||
| this.patterns = patterns; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing defensive copy for patterns array in the second constructor
This constructor directly assigns the input patterns array without creating a defensive copy. This could lead to unexpected behavior if the caller modifies the array after creating the FilterableAttribute.
- this.patterns = patterns;
+ this.patterns = Arrays.copyOf(patterns, patterns.length); // defensive copyThe third constructor correctly creates a defensive copy, but this constructor should do the same to maintain consistent encapsulation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public FilterableAttribute(String[] patterns) { | |
| // Special case of '_geo' pattern will apply special conditions to default attributes | |
| boolean patternHasGeo = false; | |
| for (String s : patterns) | |
| if (_GEO.equals(s)) { | |
| patternHasGeo = true; | |
| break; | |
| } | |
| this.facetSearch = patternHasGeo; | |
| this.filter = new HashMap<>(); | |
| this.filter.put("equality", true); | |
| this.filter.put("comparison", patternHasGeo); | |
| this.patterns = patterns; | |
| } | |
| public FilterableAttribute(String[] patterns) { | |
| // Special case of '_geo' pattern will apply special conditions to default attributes | |
| boolean patternHasGeo = false; | |
| for (String s : patterns) | |
| if (_GEO.equals(s)) { | |
| patternHasGeo = true; | |
| break; | |
| } | |
| this.facetSearch = patternHasGeo; | |
| this.filter = new HashMap<>(); | |
| this.filter.put("equality", true); | |
| this.filter.put("comparison", patternHasGeo); | |
| this.patterns = Arrays.copyOf(patterns, patterns.length); // defensive copy | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/meilisearch/sdk/model/FilterableAttribute.java between
lines 29 and 42, the constructor assigns the input patterns array directly to
the instance variable without making a defensive copy. To fix this, create a new
array and copy the contents of the input patterns array into it before assigning
it to the instance variable. This prevents external modifications to the array
from affecting the internal state and ensures consistent encapsulation like the
third constructor.
…erable-Attributes-Methods' into enhancement/meilisearch#838-Filterable-Attributes-Methods
Pull Request - Add new FiltrationAttributes POJO and update implementation of Settings, Index.
Related issue
Fixes #838
What does this PR do?
New Additions
FilterableAttributeis a flattened POJO implementation of the new granular filtration feature. It contains backwards compatibility with legacy data.GsonFilterableAttributeSerializerallows for the serialization/deserialization of these POJOs from legacy primitive string filter attributes, as well as the persistence of new single-string attributes with default settings in legacy format.Changes
GsonJsonHandlerregisters our new serializer.Index:getFilterableAttributesSettingsis updated to accommodate the new POJO. Backwards compatiblelegacyGetFilterableAttributesSettingswhich returns the traditionalString[]attribute configuration has been added as well.updateFilterableAttributeshas been updated to acceptObject[], as polymorphic backwards compatibility which can acceptnullvalues, requires no ambiguity; as we cannot do a polymorphic method, this seemed the best fit.Settings:filterableAttributeswas updated to now be of typeFilterableAttribute[].SettingsHandler:updateFilterableAttributesSettingsnow accepts aFilterableAttribute[]object.getFilterableAttributesSettingsnow returns one likewise.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation