Add support for foreignKeys settings#1262
Open
addy790 wants to merge 2 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Meilisearch document relations support via a new ChangesForeign keys settings support
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meilisearch/index.py (1)
1210-1257: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
update_settings()docstring omitsforeignKeys.The PR objectives state the
update_settings()docstring was updated to includeforeignKeysas a supported key, but the supported-settings list here still doesn't mention it.📝 Proposed fix
- 'searchCutoffMs': Maximum search time in milliseconds - 'proximityPrecision': Precision for proximity ranking - 'localizedAttributes': Settings for localized attributes + - 'foreignKeys': Cross-index document relations (experimental)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meilisearch/index.py` around lines 1210 - 1257, The update_settings() docstring is missing the new supported settings key foreignKeys in the “body” section. Update the supported-settings list in update_settings() to mention foreignKeys alongside the other settings, keeping the rest of the docstring consistent with the API docs and the existing symbols in Index.update_settings.
🧹 Nitpick comments (1)
tests/settings/test_settings_foreign_keys_meilisearch.py (1)
12-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing task-status assertion for consistency.
Unlike
test_update_foreign_keys_to_noneandtest_reset_foreign_keysin this same file, this test doesn't assertupdate.status == "succeeded"after the update, so a silently-failed task would surface as a less informative assertion failure later.✅ Proposed fix
index = empty_index() response = index.update_foreign_keys(FOREIGN_KEYS) - index.wait_for_task(response.task_uid) + update = index.wait_for_task(response.task_uid) + assert update.status == "succeeded" get_keys = index.get_foreign_keys()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/settings/test_settings_foreign_keys_meilisearch.py` around lines 12 - 20, The foreign keys update test is missing the same task-status check used by the other tests in this file. In test_update_foreign_keys, capture the result of index.update_foreign_keys(FOREIGN_KEYS), wait for response.task_uid, then assert the returned task/update status is succeeded before verifying get_foreign_keys so failures surface clearly and consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@meilisearch/index.py`:
- Around line 1210-1257: The update_settings() docstring is missing the new
supported settings key foreignKeys in the “body” section. Update the
supported-settings list in update_settings() to mention foreignKeys alongside
the other settings, keeping the rest of the docstring consistent with the API
docs and the existing symbols in Index.update_settings.
---
Nitpick comments:
In `@tests/settings/test_settings_foreign_keys_meilisearch.py`:
- Around line 12-20: The foreign keys update test is missing the same
task-status check used by the other tests in this file. In
test_update_foreign_keys, capture the result of
index.update_foreign_keys(FOREIGN_KEYS), wait for response.task_uid, then assert
the returned task/update status is succeeded before verifying get_foreign_keys
so failures surface clearly and consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 783f5435-fcce-49e4-a183-c1a48f4ab4be
📒 Files selected for processing (5)
.code-samples.meilisearch.yamlmeilisearch/config.pymeilisearch/index.pytests/conftest.pytests/settings/test_settings_foreign_keys_meilisearch.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Related issue
Fixes #1258
What does this PR do?
This PR adds support for the experimental
foreignKeyssetting.Changes
get_foreign_keys(),update_foreign_keys(), andreset_foreign_keys()methods to theIndexclass.foreign-keyspath toConfig.Paths.foreignKeysas a supported key in theupdate_settings()docstring.tests/settings/test_settings_foreign_keys_meilisearch.pycovering get, update, update-to-null, and reset, using a newenable_foreign_keysfixture to opt in to the experimental feature.get_foreign_keys_setting_1,update_foreign_keys_setting_1, andreset_foreign_keys_setting_1code samples to.code-samples.meilisearch.yaml.Testing
363 passed, 5 skipped).ruff check.ruff format.mypy.yamllinton the modified YAML file.PR checklist
Please check if your PR fulfills the following requirements:
-AI disclosure:
I used ChatGPT to help understand the repository structure, contribution workflow, troubleshoot development issues, and review the implementation. I verified the generated suggestions, tested the implementation locally, and reviewed the final changes before submitting this PR.
Summary by CodeRabbit
null, and reset-to-default behavior.