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

Making indexes generic over the contained document type #143

Open
Mubelotix opened this issue Jun 10, 2021 · 9 comments
Open

Making indexes generic over the contained document type #143

Mubelotix opened this issue Jun 10, 2021 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@Mubelotix
Copy link
Collaborator

I think the way this library handles Document types can be improved.

Current implementation

The type of the Document must be explicitly specified at each request.

let books = client.get_index("books").await?;
let results = books.search().with_query("harry pottre").execute::<Book>().await?;

That is very flexible but do we really need flexibility here? The type of the document will (and should) almost never change at runtime.
The current solution may lead to bugs where the actual type of the Index does not match the specified type of the request.

let index = client.get_index("books").await?;
let book_results = index.search().with_query("harry pottre").execute::<Book>().await?;

// some code

let movie_results = index.search().with_query("harry pottre").execute::<Movie>().await?; // Bug! The programmer forgot that the index is containing books and there will be errors at runtime.

Solution

Index should be generic over its contained Document type. That would force the programmer to specify the type of the documents at the index creation.

let books = client.get_index::<Book>("books").await?;
let results = books.search().with_query("harry pottre").execute().await?;

That makes the bug described above impossible and takes advantage of Rust's type checks and inference.
Note: We could also add an execute_dynamic method allowing the programmer to overwrite the type of the index (for this request only).

@Mubelotix Mubelotix self-assigned this Jun 10, 2021
@ppamorim
Copy link
Contributor

ppamorim commented Jun 15, 2021

@Mubelotix Could be better to create a struct Book that impl a trait Document to retrieve the key from Meilisearch? Like this?

From the library:

trait Document {
  fn key(self) -> &str;
}

From the project:

struct Book {
  let foo: String
}

impl Document for Book {
  fn key(self) -> &str {
    return "books"
  }
}

It's possible to automatically do:

let books = client.get_index::<Book>().await?;

@Mubelotix
Copy link
Collaborator Author

@ppamorim Interesting idea, but that means you would not be able to have multiple indexes containing the same document type.

@ppamorim
Copy link
Contributor

ppamorim commented Jun 15, 2021

@Mubelotix For this case, make get_index to accept a custom index name and ignore the key from the trait. This case below makes sense for me:

let books = client.get_index::<Book>("Pedro_Book").await?;

Since it has potential to be a exceptional circumstance.

@Mubelotix
Copy link
Collaborator Author

Yes that would work but I think that adding this method to the Document trait is an unnecessary abstraction. It breaks the object hierarchy: an Index contains Documents, a Document cannot choose its Index. A Document should always be handled by its Index and does not need to be aware of its name.

@ppamorim
Copy link
Contributor

ppamorim commented Jun 15, 2021

If this could be possible or anything similar...

let books = client.get_index::<Book::with_key("Pedro_Book")>().await?;

By retrieving the key of the Book above, it returns Pedro_Book.

@Mubelotix
Copy link
Collaborator Author

That would result in a compiler error. Rust expects a type but this is a function call.

@ppamorim
Copy link
Contributor

I know, that's a hypothetical case if we could do this in Rust.

@ppamorim
Copy link
Contributor

But I still think that the developer should duplicate the structs and name them correctly, it doesn't matter if the struct is similar to another struct. Later on this can be a problem.
I have this same situation in my code.

@curquiza
Copy link
Member

Hello @Mubelotix,
I checked with our CTO, your solution seems to be a good idea from his point of you! Having the type when creating the index is better.

@bidoubiwa bidoubiwa added enhancement New feature or request hacktoberfest labels Sep 27, 2022
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