-
Notifications
You must be signed in to change notification settings - Fork 134
Add v1.14 filterable attributes #927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds granular, object-based filterable attributes (models, serializers/deserializers), index-level granular getters/setters, legacy adapter and exception for String[] compatibility, TaskDetails normalization, test coverage, and a new AGENTS.md developer doc. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Index
participant SettingsHandler
participant HTTP as HTTP Client
participant Serializer as Gson/Jackson
participant Model as FilterableAttributesConfig[]
rect rgba(0, 100, 150, 0.5)
Note over Client,Model: Retrieve granular filterable attributes
Client->>Index: getGranularFilterableAttributesSettings()
Index->>SettingsHandler: getGranularFilterableAttributesSettings(uid)
SettingsHandler->>HTTP: GET /indexes/{uid}/settings/filterable-attributes
HTTP->>Serializer: response body
Serializer->>Model: deserialize mixed syntax -> FilterableAttributesConfig[]
Model-->>SettingsHandler: configs
SettingsHandler-->>Index: configs
Index-->>Client: configs
end
sequenceDiagram
participant Client
participant Index
participant SettingsHandler
participant Adapter as FilterableAttributesLegacyAdapter
participant HTTP as HTTP Client
participant Serializer as Gson/Jackson
rect rgba(150, 100, 0, 0.5)
Note over Client,Adapter: Update using legacy String[] (back-compat)
Client->>Index: updateFilterableAttributes(String[] names)
Index->>SettingsHandler: updateFilterableAttributes(uid, names)
SettingsHandler->>Adapter: fromLegacyNames(names) -> FilterableAttributesConfig[]
SettingsHandler->>HTTP: PUT /indexes/{uid}/settings/filterable-attributes (body: configs)
HTTP->>Serializer: serialize configs
HTTP-->>SettingsHandler: TaskInfo
SettingsHandler-->>Index: TaskInfo
Index-->>Client: TaskInfo
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/meilisearch/sdk/Index.java (1)
846-857: Fix Javadoc return type (nowFilterableAttributesConfig[]).
The comment still states “as String”, which no longer matches the method signature.✏️ Proposed doc fix
- * `@return` filterable attributes of a given uid as String + * `@return` filterable attributes of a given uid as FilterableAttributesConfig[]
🤖 Fix all issues with AI agents
In `@src/main/java/com/meilisearch/sdk/json/JacksonJsonHandler.java`:
- Around line 29-36: The custom ObjectMapper constructor in JacksonJsonHandler
currently skips registering the FilterableAttributesConfig (de)serializer
module, causing polymorphic string/object formats to fail; update the
custom-constructor code path in class JacksonJsonHandler to create and register
the same SimpleModule (adding JacksonFilterableAttributesConfigSerializer and
JacksonFilterableAttributesConfigDeserializer for FilterableAttributesConfig) on
the mapper instance and call mapper.registerModule(...) so both constructors
configure the mapper identically.
🧹 Nitpick comments (4)
AGENTS.md (1)
5-5: Optional wording tweak for clarity.Consider replacing “speed up” with a stronger verb (e.g., “accelerate”) to tighten the sentence.
src/main/java/com/meilisearch/sdk/json/JacksonFilterableAttributesConfigSerializer.java (1)
11-77: Consider explicit null handling when serializing attributePatterns.If null entries are possible (the deserializer preserves them), being explicit avoids relying on
writeString(null)behavior.♻️ Optional defensive handling
- for (String pattern : patterns) { - gen.writeString(pattern); - } + for (String pattern : patterns) { + if (pattern == null) { + gen.writeNull(); + } else { + gen.writeString(pattern); + } + }Please confirm null-string handling with the Jackson version in use.
src/main/java/com/meilisearch/sdk/json/JacksonFilterableAttributesConfigDeserializer.java (1)
19-43: Optional guard for JSON null values.If Jackson ever routes VALUE_NULL through this deserializer, returning null is safer than creating an empty config.
♻️ Optional defensive guard
- JsonNode node = p.getCodec().readTree(p); + JsonNode node = p.getCodec().readTree(p); + if (node == null || node.isNull()) { + return null; + }Please confirm null-token handling with the Jackson version in use.
src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributesConfigTypeAdapter.java (1)
110-118: Potential issue: Missing null handling when reading array elements.If the API ever returns a
nullinside theattributePatternsarray,reader.nextString()will throw anIllegalStateException. Consider handlingJsonToken.NULLwithin the loop for defensive coding.♻️ Suggested defensive fix
private String[] readAttributePatterns(JsonReader reader) throws IOException { List<String> patterns = new ArrayList<>(); reader.beginArray(); while (reader.peek() != JsonToken.END_ARRAY) { - patterns.add(reader.nextString()); + if (reader.peek() == JsonToken.NULL) { + reader.nextNull(); + patterns.add(null); + } else { + patterns.add(reader.nextString()); + } } reader.endArray(); return patterns.toArray(new String[0]); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/java/com/meilisearch/sdk/json/JacksonJsonHandlerTest.java`:
- Around line 73-135: The test file JacksonJsonHandlerTest has Spotless
formatting issues; run the formatter and commit the changes. Fix by running
./gradlew spotlessApply to auto-format the file, verify the formatting changes
in the JacksonJsonHandlerTest class (including methods
granularFilterableAttributesRoundTripWithCustomMapper and
settingsWithGranularFilterableAttributesRoundTripWithCustomMapper), stage the
updated file, and re-commit so spotlessJavaCheck passes in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 5-11: Update the example Docker commands so they match the
guidance to persist the Gradle cache: add the -v gradle-cache:/root/.gradle
volume flag to each `docker compose run --rm package ...` example (the lint,
`./gradlew test`, and `./gradlew test integrationTest` commands) so users
running the provided examples actually mount the Gradle cache as recommended.
🧹 Nitpick comments (1)
AGENTS.md (1)
9-9: Simplify redundant bash invocation.The command
bash -c "bash ./scripts/lint.sh"invokes bash twice unnecessarily.♻️ Simplify to direct execution
-- `docker compose run --rm package bash -c "bash ./scripts/lint.sh"` - lint code +- `docker compose run --rm package bash ./scripts/lint.sh` - lint code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Strift thanks for the PR
can we still use the old way of using filterable attribute? Is this PR breaking?
settings.setFilterableAttributes(new String[] {"genres", "director"});If no, can we have a way to accept both, or to create a new function to handle the new filterable attribute way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/main/java/com/meilisearch/sdk/json/GsonTaskDetailsTypeAdapterFactory.java`:
- Line 1: The file com.meilisearch.sdk.json.GsonTaskDetailsTypeAdapterFactory
has formatting violations causing spotlessJavaCheck to fail; run the formatter
(execute ./gradlew spotlessApply) or apply the project's Spotless Java style to
that class and package declaration and method bodies so the file matches the
repo's style rules, then re-run the build; ensure imports, spacing, braces, and
indentation in GsonTaskDetailsTypeAdapterFactory are corrected to satisfy CI.
In `@src/main/java/com/meilisearch/sdk/model/Settings.java`:
- Line 4: The file Settings.java has formatting violations (spotless) — run the
project formatter to fix style or manually reformat the file to match project
rules: run ./gradlew spotlessApply (or apply your IDE’s project formatter) and
ensure imports and class declaration in Settings.java (e.g., the import
lombok.AccessLevel and the Settings class) follow the repository's spotless/Java
style so the spotlessJavaCheck passes.
In `@src/main/java/com/meilisearch/sdk/model/TaskDetails.java`:
- Line 3: The TaskDetails.java file fails spotless formatting; run the project's
formatter (./gradlew spotlessApply) and reformat this file so it conforms to the
project's style rules—ensure import ordering (e.g., the JsonDeserialize import),
spacing, indentation, and annotations inside the TaskDetails class are
corrected; after applying spotless, verify the file compiles and the
spotlessJavaCheck passes before committing.
In `@src/test/java/com/meilisearch/integration/SettingsTest.java`:
- Around line 15-35: The file SettingsTest.java is failing Spotless formatting;
run the formatter and commit the fixed file: execute ./gradlew spotlessApply,
review changes in SettingsTest (look for the SettingsTest class and related
imports like Settings, TestData, AbstractIT), ensure the import block and
file-wide formatting are corrected, then add and commit the reformatted file so
CI’s spotlessJavaCheck passes.
🧹 Nitpick comments (4)
src/main/java/com/meilisearch/sdk/json/GsonTaskDetailsTypeAdapterFactory.java (1)
39-80: Consider handling the case whentreeisnull.If
elementAdapter.read(in)returnsnull(e.g., for a JSONnullliteral), the code skips normalization and passesnulldirectly todelegate.fromJsonTree(tree). While Gson's delegate adapter typically handles this gracefully, an explicit early return or null-check could make the intent clearer and avoid potential issues with different Gson versions.🔧 Optional: Add explicit null handling
`@Override` public TaskDetails read(JsonReader in) throws IOException { JsonElement tree = elementAdapter.read(in); + if (tree == null || tree.isJsonNull()) { + return null; + } - if (tree != null && tree.isJsonObject()) { + if (tree.isJsonObject()) { JsonObject obj = tree.getAsJsonObject(); // ... rest of the normalization logic } return delegate.fromJsonTree(tree); }src/main/java/com/meilisearch/sdk/json/TaskDetailsFilterableAttributesDeserializer.java (1)
34-42: Potential data loss: only the firstattributePatternsentry is preserved.When deserializing an object with multiple
attributePatterns, only the first entry is extracted. This silently discards any additional patterns, which could lead to unexpected behavior if the user expects all patterns to be represented.Consider whether this is the intended behavior for the legacy
String[]view, or if an exception/warning should be raised when multiple patterns exist (similar to howFilterableAttributesLegacyAdapter.toLegacyNamesOrThrowhandles non-simple configs)..code-samples.meilisearch.yaml (1)
571-571: Consistency check: Other samples still use legacyString[]syntax.This sample uses
setFilterableAttributes(new String[] { "mass", "_geo" })whileupdate_filterable_attributes_1now uses the granular API. This is acceptable since backwards compatibility is preserved, but consider whether documentation should note both approaches or if additional samples should demonstrate the granular API for geo/mass filtering use cases.src/main/java/com/meilisearch/sdk/model/Settings.java (1)
52-55: Document that the legacy getter can throwGranularFilterableAttributesException.The
getFilterableAttributes()method delegates totoLegacyNamesOrThrow, which throwsGranularFilterableAttributesExceptionwhen granular configs cannot be losslessly converted. Existing code calling this method may not expect an exception.Consider adding a Javadoc
@throwsclause to make this behavior explicit, and ensure thatMeilisearchExceptionis a runtime exception so this doesn't introduce a compile-time breaking change.📝 Suggested Javadoc enhancement
- /** Legacy String[] view of filterable attributes. */ + /** + * Legacy String[] view of filterable attributes. + * + * `@return` attribute names as a String array + * `@throws` GranularFilterableAttributesException if granular configs cannot be reduced to + * simple attribute names without losing information + */ public String[] getFilterableAttributes() {
src/main/java/com/meilisearch/sdk/json/GsonTaskDetailsTypeAdapterFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.code-samples.meilisearch.yaml:
- Line 248: The sample calls settings.setFilterableAttributes(...) with a
FilterableAttributesConfig[] but the API now requires
setFilterableAttributesConfig(...) for granular configs; update the call to use
settings.setFilterableAttributesConfig(new FilterableAttributesConfig[] {
genres, director }) so the method signature matches FilterableAttributesConfig[]
(keep the existing genres and director variables as-is).
In
`@src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributesConfigTypeAdapter.java`:
- Around line 110-118: The reader method readAttributePatterns currently assumes
all array elements are strings and calls reader.nextString(), which will throw
on JSON nulls; update the loop in readAttributePatterns to check reader.peek()
for JsonToken.NULL per iteration, call reader.nextNull() and add a null entry to
patterns when encountered, otherwise call reader.nextString() and add the
string—this preserves symmetry with the write path that can emit nulls and
ensures null array elements are handled safely.
In
`@src/main/java/com/meilisearch/sdk/json/JacksonFilterableAttributesConfigDeserializer.java`:
- Around line 24-43: The deserializer currently silently accepts
non-textual/non-object root nodes and coerces invalid pattern entries; update
JacksonFilterableAttributesConfigDeserializer (deserialize method) to fail fast
using ctxt.reportInputMismatch(...) when the root JsonNode is null or not
textual and not an object, when attributePatterns contains a non-null
non-textual entry, and when features is present but not an object; keep the
existing handling for a textual node (return
FilterableAttributesConfig.simple(node.asText())), but replace silent ignores
with explicit ctxt.reportInputMismatch calls that include clear messages
referencing "attributePatterns" and "features" so invalid input types are
rejected rather than coerced.
In `@src/main/java/com/meilisearch/sdk/model/FilterableAttributesConfig.java`:
- Around line 1-34: Spotless formatting violations were detected in
FilterableAttributesConfig; run the formatter and commit the changes: execute
./gradlew spotlessApply (or reformat the file to match project Spotless rules)
and ensure the class FilterableAttributesConfig, its fields attributePatterns
and features, and the simple(String) factory method follow the repository's Java
formatting (imports, spacing, indentation, and trailing newlines) before
committing the updated file.
In
`@src/main/java/com/meilisearch/sdk/model/FilterableAttributesLegacyAdapter.java`:
- Around line 1-63: The file FilterableAttributesLegacyAdapter has Spotless
formatting violations; run ./gradlew spotlessApply (or apply your project's
Spotless formatter) and reformat this class, ensuring the formatting of the
class declaration, imports, Javadoc, and the methods fromLegacyNames and
toLegacyNamesOrThrow complies with the project's style rules; after
auto-applying formatting, re-run the build to verify the Spotless check passes.
src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributesConfigTypeAdapter.java
Show resolved
Hide resolved
src/main/java/com/meilisearch/sdk/json/JacksonFilterableAttributesConfigDeserializer.java
Show resolved
Hide resolved
src/main/java/com/meilisearch/sdk/model/FilterableAttributesLegacyAdapter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/meilisearch/integration/SettingsTest.java (1)
1129-1148: Avoid order-sensitive equality on reset filterable attributes.Filterable attribute order isn’t guaranteed; comparing ordered lists at Line 1146 can be flaky. Use order-insensitive matching (or sort both arrays) like the other filterable-attribute tests.
✅ Suggested assertion tweak
- assertThat( - Arrays.asList(resetFilterableAttributes), - is(equalTo(Arrays.asList(initialFilterableAttributes)))); + assertThat( + Arrays.asList(resetFilterableAttributes), + containsInAnyOrder(initialFilterableAttributes));
🤖 Fix all issues with AI agents
In
`@src/main/java/com/meilisearch/sdk/json/GsonTaskDetailsTypeAdapterFactory.java`:
- Around line 58-70: GsonTaskDetailsTypeAdapterFactory currently only keeps the
first entry of the "attributePatterns" array when normalizing, losing additional
patterns; update the normalization logic in GsonTaskDetailsTypeAdapterFactory to
iterate over the entire patterns JsonArray and add each element (using
JsonNull.INSTANCE for nulls) to the normalized JsonArray so all patterns are
preserved; apply the same change in TaskDetailsFilterableAttributesDeserializer
to flatten and collect every entry from the "attributePatterns" array into the
resulting String[] (handling nulls appropriately) so both Gson and Jackson
deserializers retain all server-provided patterns.
src/main/java/com/meilisearch/sdk/json/GsonTaskDetailsTypeAdapterFactory.java
Outdated
Show resolved
Hide resolved
… handling in deserialization
|
Hey @curquiza @brunoocasali, I updated the PR:
I updated the PR description, consult it for more information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributesConfigTypeAdapter.java`:
- Around line 110-161: These three deserialization helpers
(readAttributePatterns, readFeatures, readFilter) must guard against JSON null
tokens to avoid IllegalStateException: at the start of each method check if
reader.peek() == JsonToken.NULL, call reader.nextNull() and return null (for
readAttributePatterns return null instead of an array; for readFeatures and
readFilter return null), otherwise proceed to call
reader.beginArray()/beginObject(); update readAttributePatterns, readFeatures,
and readFilter accordingly.
src/main/java/com/meilisearch/sdk/json/GsonFilterableAttributesConfigTypeAdapter.java
Show resolved
Hide resolved
| client.index("movies").getGranularFilterableAttributesSettings(); | ||
| // Legacy String[] API | ||
| client.index("movies").getFilterableAttributesSettings(); | ||
| update_filterable_attributes_1: |- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an example with updateGranularFilterableAttributesSettings? 😇
| director.setFeatures(directorFeatures); | ||
|
|
||
| // Update settings | ||
| client.index("movies").updateGranularFilterableAttributesSettings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@curquiza updated the example to use updateGranularFilterableAttributesSettings
curquiza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!!! Muchas gracias!!!
Pull Request
Related issues
Fixes #924
Fixes #838
What this PR does
Introduce Granular Filterable Attributes (v1.14+) with full backward compatibility
FilterableAttributesConfigto support per-attribute settings for facet search and filter options (equality/comparison).String[]method signatures inIndexandSettingsas legacy adapters over the new internal model.GranularFilterableAttributesExceptionif granular features are active, providing a clear migration path instead of a JSON parsing failure.updateGranularFilterableAttributesSettingsand related methods for users opting into the advanced configuration features.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.