-
Notifications
You must be signed in to change notification settings - Fork 128
Add disableOnNumbers setting in TypoTolerance #885
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
base: main
Are you sure you want to change the base?
Add disableOnNumbers setting in TypoTolerance #885
Conversation
WalkthroughAdds support for the disableOnNumbers field in TypoTolerance, introduces an integration test verifying its update behavior, and adds a corresponding code sample entry typo_tolerance_guide_5 in the Meilisearch YAML code samples. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (2)
.code-samples.meilisearch.yaml (1)
455-458
: Docs sample looks good; minor consistency nit on index naming.Most samples use "movies"/"books". If "1917" isn’t intentional to emphasize numeric terms, prefer staying consistent with other examples.
Apply this minimal change if consistency is preferred:
- client.index("1917").updateTypoToleranceSettings(typoTolerance); + client.index("movies").updateTypoToleranceSettings(typoTolerance);src/test/java/com/meilisearch/integration/SettingsTest.java (1)
903-919
: Good coverage for disableOnNumbers; consider also asserting the toggle back to false.The test validates default false and setting true. Adding a final toggle back to false ensures both directions of the update work.
Apply this diff to extend the test:
TypoTolerance updatedTypoToleranceDisableOnNumbers = index.getTypoToleranceSettings(); assertThat(updatedTypoToleranceDefault.isDisableOnNumbers(), is(equalTo(false))); assertThat(updatedTypoToleranceDisableOnNumbers.isDisableOnNumbers(), is(equalTo(true))); + + // Toggle back to false and assert + TypoTolerance newTypoToleranceBackToFalse = new TypoTolerance().setDisableOnNumbers(false); + index.waitForTask(index.updateTypoToleranceSettings(newTypoToleranceBackToFalse).getTaskUid()); + TypoTolerance updatedTypoToleranceBackToFalse = index.getTypoToleranceSettings(); + assertThat(updatedTypoToleranceBackToFalse.isDisableOnNumbers(), is(equalTo(false)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.code-samples.meilisearch.yaml
(1 hunks)src/main/java/com/meilisearch/sdk/model/TypoTolerance.java
(1 hunks)src/test/java/com/meilisearch/integration/SettingsTest.java
(1 hunks)
@@ -19,6 +19,7 @@ public class TypoTolerance { | |||
protected HashMap<String, Integer> minWordSizeForTypos; | |||
protected String[] disableOnWords; | |||
protected String[] disableOnAttributes; | |||
protected boolean disableOnNumbers; |
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.
💡 Verification agent
🧩 Analysis chain
New field added correctly; verify partial-update serialization does not unintentionally reset values.
The addition of disableOnNumbers is aligned with the v1.15 API and Lombok will generate isDisableOnNumbers()/setDisableOnNumbers(boolean) as expected. One caveat: using a primitive boolean means new TypoTolerance() will serialize disableOnNumbers=false by default. If the update endpoint applies PATCH semantics, this could unintentionally reset a previously true value when callers intend to modify other fields only.
Please confirm how we serialize TypoTolerance (Gson/Jackson config) and whether default-valued booleans are included in the payload. If they are, consider documenting this behavior prominently or introducing a mechanism to omit default-valued fields on partial updates.
Run this script to locate the serializer config and the update method:
I can propose a safe approach (e.g., selectively omitting default-valued fields during serialization) once we confirm the serializer behavior.
🏁 Script executed:
#!/bin/bash
# Find how TypoTolerance updates are serialized and sent
rg -n -A 8 -B 8 $'updateTypoToleranceSettings\\('
rg -n -A 4 -B 4 'GsonBuilder|new Gson|ObjectMapper|Jackson'
rg -n -A 4 -B 4 'TypoTolerance\\s+\\{' src/main/java
Length of output: 66323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
src/main/java/com/meilisearch/sdk/json/JacksonJsonHandler.java
src/main/java/com/meilisearch/sdk/json/GsonJsonHandler.java
src/main/java/com/meilisearch/sdk/json/JsonHandler.java
src/main/java/com/meilisearch/sdk/HttpClient.java
src/main/java/com/meilisearch/sdk/http/BasicRequest.java
src/main/java/com/meilisearch/sdk/SettingsHandler.java
src/main/java/com/meilisearch/sdk/model/TypoTolerance.java
)
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "==== $f ===="
sed -n '1,240p' "$f" || true
echo
else
echo "==== $f MISSING ===="
fi
done
Length of output: 23765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# locate BasicRequest, HttpRequest, CustomOkHttpClient and where body is serialized
rg -n --hidden "class BasicRequest|interface BasicRequest|class HttpRequest|class CustomOkHttpClient|class CustomOkHttp" || true
rg -n --hidden "create\\(HttpMethod\\.PATCH|create\\(HttpMethod\\.POST|toJson\\(|writeValueAsString\\(" || true
# show files if found
files_to_show=(
"$(rg -n --files | rg 'BasicRequest' || true)"
"$(rg -n --files | rg 'HttpRequest' || true)"
"$(rg -n --files | rg 'CustomOkHttpClient' || true)"
src/main/java/com/meilisearch/sdk/http/request/BasicRequest.java
src/main/java/com/meilisearch/sdk/http/request/HttpRequest.java
src/main/java/com/meilisearch/sdk/http/CustomOkHttpClient.java
src/main/java/com/meilisearch/sdk/http/CustomOkHttp.java
)
for f in "${files_to_show[@]}"; do
if [ -n "$f" ] && [ -f "$f" ]; then
echo "==== $f ===="
sed -n '1,240p' "$f" || true
echo
else
echo "==== $f MISSING ===="
fi
done
Length of output: 13606
New field added correctly; verify partial-update serialization does not unintentionally reset values.
Short summary: both JSON handlers in this repo will serialize primitive booleans, so new TypoTolerance() will produce "disableOnNumbers": false in PATCH payloads and can overwrite a previously true server value.
Files to review / action items:
- src/main/java/com/meilisearch/sdk/model/TypoTolerance.java — current field:
- protected boolean disableOnNumbers;
- Recommend changing to a nullable wrapper so absent = omitted in JSON:
- Diff:
-
- protected boolean disableOnNumbers;
-
- protected Boolean disableOnNumbers;
-
- Diff:
- src/main/java/com/meilisearch/sdk/json/JacksonJsonHandler.java — encode() sets mapper.setSerializationInclusion(Include.NON_NULL) (nulls are omitted, but primitive false is not).
- src/main/java/com/meilisearch/sdk/json/GsonJsonHandler.java — uses default GsonBuilder (nulls are omitted by default, but primitive false is serialized).
- src/main/java/com/meilisearch/sdk/http/request/BasicRequest.java and src/main/java/com/meilisearch/sdk/SettingsHandler.java — request.create(...) calls jsonHandler.encode(...) and SettingsHandler.updateTypoToleranceSettings passes the TypoTolerance object to PATCH, so the boolean will be sent.
Recommendations (short):
- Preferred: change disableOnNumbers to Boolean and update tests that assume a default false so partial updates omit the field unless explicitly set.
- Alternatives: document the current behavior clearly, or implement selective serialization for partial updates (e.g., custom serializer, or use Include.NON_DEFAULT for Jackson — but that has wider impact).
🤖 Prompt for AI Agents
src/main/java/com/meilisearch/sdk/model/TypoTolerance.java around line 22:
change the primitive field to a nullable wrapper and update usages so
partial-update PATCH payloads omit the field unless explicitly set. Replace
"protected boolean disableOnNumbers;" with "protected Boolean
disableOnNumbers;", update constructors/getters/setters and any code/tests that
assume a default false to handle null, and ensure both JSON handlers do not
force-serialize nulls (verify Jackson uses Include.NON_NULL and that
GsonJsonHandler does not call serializeNulls or otherwise enable null
serialization); run/adjust tests that depended on primitive default values.
Pull Request
Related issue
Fixes #871
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Documentation
Tests