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

Questions around API design for Client's generate_tenant_token method's UID parameter #488

Open
moy2010 opened this issue Jul 3, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@moy2010
Copy link
Contributor

moy2010 commented Jul 3, 2023

Description
I would like to open a discussion on the API design decision around the Client's generate_tenant_token method's UID parameter. Since the Discussions feature is not enabled for this repo, I had to open it as an issue instead.

Currently in the SDK v0.24.1 the Client's generate_tenant_token methos is implemented as follows:

pub fn generate_tenant_token(
        &self,
        api_key_uid: String,
       ...
}

By accepting a String, as an API consumer what this signature tells me is that I can pass any string which would uniquely identify the tenant token to be generated.

Unfortunately, this assumption is broken downstream by the following implementation detail in the tenant_tokens module:

// Validate uuid format
pub fn generate_tenant_token(
    api_key_uid: String,
    ...
) -> Result<String, Error> {
    let uid = Uuid::try_parse(&api_key_uid)?;

Here the api_key_uid is parsed into a Uuid and, if the parsing fails, the whole tenant token generation will fail with a Uuid error.

Expected behavior
A more coherent API experience. The API should clearly communicate to the consumers what does it expect from them.

Current behavior
If a non-Uuid string is passed, an error is returned.

Environment (please complete the following information):

  • OS: [e.g. Debian GNU/Linux]
  • Meilisearch version: n/a
  • meilisearch-rust version: v0.24.1
@bidoubiwa
Copy link
Contributor

Hey @moy2010, you're right about the unclear usage. How would you handle this?

@bidoubiwa bidoubiwa added the enhancement New feature or request label Jul 6, 2023
@moy2010
Copy link
Contributor Author

moy2010 commented Jul 6, 2023

How would you handle this?

I would create a NewType wrapper around Uuid (i.e. ApiKeyUid(Uuid)), and use it across the different methods that interact with this entity.

@postmeback
Copy link
Contributor

postmeback commented Jul 7, 2024

Validating the UUID format is already implemented.

image

I believe this issue can also be closed.

@irevoire
Copy link
Member

Hey @postmeback, what @moy2010 was saying is that it would be nice to take the Uuid directly in parameter instead of parsing the Uuid later on.
This way, as a client, you cannot write something wrong; you're guaranteed to use a valid Uuid that was returned from Meilisearch.

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

No branches or pull requests

4 participants