Support user dictionary loading #870
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #870 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 13 13
Lines 721 730 +9
=========================================
+ Hits 721 730 +9
☔ View full report in Codecov by Sentry. |
meilisearch/index.py
Outdated
| """ | ||
| return self.http.get(self.__settings_url_for(self.config.paths.dictionary)) | ||
|
|
||
| def update_dictionary(self, body: Union[List[str], None]) -> TaskInfo: |
There was a problem hiding this comment.
Body is required here.
| def update_dictionary(self, body: Union[List[str], None]) -> TaskInfo: | |
| def update_dictionary(self, body: List[str]) -> TaskInfo: |
There was a problem hiding this comment.
Alright, I pushed this change.
Is there some reference that could have allowed me to prevent this? From what I could see most other List settings allowed None, I'm not too experienced with Python so I tried not to do my own thing.
There was a problem hiding this comment.
If it is something that has been released like this you can find it in the API reference. In this case here. If it could be None there will be a default value, something like this.
If it is pre-release they create an issue in the integration-guide repo where you can find information about the needed changes. Something like this.
There was a problem hiding this comment.
I apologize for going on about this, but I wanted to get it straightened out before writing the text separator subroutes.
This (null update) request actually successfully clears the dictionary for a 'books' index:
curl \
-X PUT 'http://localhost:7700/indexes/books/settings/dictionary' \
-H "Content-Type: application/json" --data-binary "null"
{"taskUid":66184,"indexUid":"books","status":"enqueued","type":"settingsUpdate","enqueuedAt":"2023-10-24T12:09:45.811281464Z"}That request is what would be sent since
# _http_requests.py > send_request
data=json.dumps(body) if body else "" if body == "" else "null",represents None as null in the payload. Dictionary does in fact have a default value in the API documentation:
Reset dictionary
DELETE/indexes/{index_uid}/settings/dictionaryReset an index's dictionary to its default value, [].
There was a problem hiding this comment.
Fair enough, I guess technically it works but it wouldn't 100% do what the user expects. [] and null aren't the same thing, so if they want to remove the values they have added they should really be using the reset route.
There was a problem hiding this comment.
I agree, reset properly communicates its intentions (or update with [] for that matter), besides a variable being unexpectedly null is a very common programming mistake which would result in clearing this whole setting.
Should we create an issue to also remove the None option for other similar settings?
There was a problem hiding this comment.
In a lot of cases the None does make sense. Taking the Index create method as an example, options: Optional[Dict[str, Any]] = None makes sense because None in this case means not to attach any addtional options. In other words, some routes can take additional parameters but don't have to. So None in this case means there are no additional options to attach to the request.
In this case, even if null gets set to the server, the user is saying they want to take the defaults by not attaching additional parameters so they get what they expect.
There was a problem hiding this comment.
I suppose the distinction would be whether or not it makes sense to call the function without that argument. Index create can reasonably be called without options and use its defaults, however:
def update_searchable_attributes(self, body: Union[List[str], None]) -> TaskInfo:calling this method with a body of None will set searchable_attributes to all ([*]).
There was a problem hiding this comment.
Your distinction is correct to me...but I found some context around why they are set like they are. #762 (comment).
So while I think the way we have gone about this PR makes the most sense to me, I suppose we should allow the None option as you originally had it to be consistant with what the team wants. Sorry about that
|
Codecov seems to be broken again |
|
bors merge |
|
Build succeeded:
|
This reverts commit 52d352d. See: meilisearch#870 (comment)
872: Revert dictionary parameter back to optional r=sanders41 a=ellnix This reverts commit 52d352d. See: #870 (comment) Co-authored-by: ellnix <103502144+ellnix@users.noreply.github.com>
Pull Request
Related issue
Fixes #853
PR checklist
Please check if your PR fulfills the following requirements: