Skip to content
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

Create key: Add ability to specify the Uid and Name value #342

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

dbolkensteyn
Copy link
Contributor

@dbolkensteyn dbolkensteyn commented Aug 22, 2022

type Key struct {} and type KeyParsed struct {} both have a Key string field.

However, KeyParsed's one is never filled and thus the server always will generate a random key, even if a value was specified in the call to client.CreateKey(&meilisearch.Key{Key: "..."}}.

Copy link
Contributor

@alallema alallema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dbolkensteyn,
Thanks for your PR. Indeed, the key is not specified because convertKeyToParsedKey() is called only by the CreateKey() method, and no matter what key is passed to that method, it will create a new key. So yes it will always return a random new key.

@dbolkensteyn
Copy link
Contributor Author

Oh right, key is not present as a parameter in the REST API apparently: https://docs.meilisearch.com/reference/api/keys.html#create-a-key

That's a bummer since it makes it impossible to create a predictable search key.

@dbolkensteyn
Copy link
Contributor Author

Hey @alallema, could you please review this once more?

They Key parameter isn't part of the REST API, but UID is. Since Key is derived from the master key and UID, it becomes predictable, thus solving my original issue.

This PR now sends both UID and Name (which seemed to have been forgotten) to the REST API. I've also updated the corresponding unit test.

@alallema
Copy link
Contributor

Hey @alallema, could you please review this once more?

They Key parameter isn't part of the REST API, but UID is. Since Key is derived from the master key and UID, it becomes predictable, thus solving my original issue.

This PR now sends both UID and Name (which seemed to have been forgotten) to the REST API. I've also updated the corresponding unit test.

Hi @dbolkensteyn ,
Yes! I want to tell you that but I wasn't working yesterday sorry for the delay. I even find you the specification in case.

@alallema alallema changed the title Create key: Add ability to specify the key's value Create key: Add ability to specify the Uid and Name value Aug 24, 2022
@alallema alallema added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Aug 24, 2022
Copy link
Contributor

@alallema alallema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @dbolkensteyn for this PR and for contributing to Meilisearch ❤️
Good catch and at the same time you fix the fact that the Name of the Key wasn't created 🔥
LGTM!

@alallema
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 24, 2022

@bors bors bot merged commit cee4456 into meilisearch:main Aug 24, 2022
@dbolkensteyn
Copy link
Contributor Author

Awesome thank you so much @alallema

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants