SearchResult is badly shaped

[juchom]

Description
The API is a bit weird when searching documents.

The C# api when searching returns an ISearchable<T> which is implemented by SearchResult<T> and by PaginatedSearchResult<T>.

The Meilisearch Server response has only one format according to the doc here : https://www.meilisearch.com/docs/reference/api/search#response

Is there any reason to have the C# client designed like this ?

This raise many issues :

• As a client I'm expecting to have a real object and not an interface.
• If the interface is the right way, how as a client am I supposed to upcast this to the right object in order to access all the fields ?

Expected behavior

The server is sending only one type of response, so we should only have a single object matching the response.

The interface ISearchable<T>, the class PaginatedSearchResult<T> and the converter ISearchableJsonConverterFactory are not needed at all.

The SearchResult<T> needs to be updated to support the Meilisearch response according to the doc.

[alallema]

Hi @juchom,
Thanks for raising this issue.
Currently, the API has two types of response depending on whether the documents are in paginated mode or not see doc. We wondered how to manage this case in the best way so that the user can easily use these two types of results and we have therefore managed this behavior by giving the possibility to retrieve two different types:
Default search:

{
	"hits": [
		...
	],
	"query": "a",
	"processingTimeMs": 3,
	"limit": 20,
	"offset": 0,
	"estimatedTotalHits": 12
}

Paginated search:

{
	"hits": [
		...
	],
	"query": "a",
	"processingTimeMs": 1,
	"hitsPerPage": 2,
	"page": 1,
	"totalPages": 6,
	"totalHits": 12
}

That's for the explanation. If you have a better alternative I'm all ears, knowing that your inputs are always very interesting and that we are open to any improvement to reduce user friction 😊

[juchom]

Thanks @alallema,

I double checked the api doc and see that both fields are present depending on how you query the server. I understand why you made this choice.

But I think, it's not a good decision, having to safely upcast an interface to its concrete implementation and then use this upcasted object is inefficient and the dev experience is not great.

According to the doc (even if it's not clear, you should add a column saying if the field in the response is optional, https://www.meilisearch.com/docs/reference/api/search#response), the api response are marking this fields as optional and this should be the same in C#.

The SearchResult<T> should contains all this properties as nullable properties and match the server contract.

[juchom]

I just checked your rust client and it's shaped with one model with optional properties :

https://github.com/meilisearch/meilisearch-rust/blob/fc1d41dbb1c03683cf49a44fdb7c9738b8b37d30/src/search.rs#L59-L86

[alallema]

I just checked your rust client and it's shaped with one model with optional properties :

Yes, the Golang too!

The SearchResult should contains all this properties as nullable properties and match the server contract.

It's very interesting, thank you. I agree with you in fact I think it would probably be easier to use with optional for the users

[brunoocasali]

Hi @juchom, thanks for raising this topic with us!

The decision to use the interface was a no-brainer to me since it seems the correct way to map this type of API in a statically typed OOP language.

In fact, I didn't consider using the C# optionals. I probably didn't remember C# had it 😄.

Before committing to this breaking change, I would like to get more context about why this isn't good. Is this bad just because this is a library? And will it be harder to make things work if you have to cast the object?

Some usage examples:

• https://github.com/meilisearch/meilisearch-dotnet/blob/main/tests/Meilisearch.Tests/SearchTests.cs#L90
• https://github.com/meilisearch/meilisearch-dotnet/blob/main/tests/Meilisearch.Tests/SearchTests.cs#L76

Because to be honest, I think it is safer to have the type-casted object compared to the nullable version. Also, we could change even more the current responses, so adding more optionals will not be that interesting (the SearchResult object will be a God Object).

[juchom]

Hi @brunoocasali

The main reasons for this are :

• Interfaces are not meant to be used like this. These objects have many properties in common, but they are just DTO. Hiding there structure behind an interface is not wise.
• This library is a client for an api, as an end user I'm expecting to have a physical object matching what the server is sending.
• What I found really akward is calling a method and the response can be of two types (which is actually not possible in C# to have the same signature and have different returns, this is why you used an interface)

I understand why you made this decision, but I think this client is not responsible for this kind of transformations.

As a end user I really expect to have an object that match the api response, other transformations / mapping can be done by the end user (by hand or using some libraries).

Also, we could change even more the current responses, so adding more optionals will not be that interesting (the SearchResult object will be a God Object).

If the api can send this fields, then the object receiving it is not supposed to lie and show them as optional to exactly match the api response.

[brunoocasali]

Got it! Thanks for the reply!

Do you have some examples of other popular SDKs with the same type of implementation/behavior? I would like to have some examples to back the decision.

[juchom]

Have a look at Algolia's SearchResponse, this is a massive object with many objects that all can be null

https://github.com/algolia/algoliasearch-client-csharp/blob/master/src/Algolia.Search/Models/Search/SearchResponse.cs

This is also how it is shaped in your rust client :

https://github.com/meilisearch/meilisearch-rust/blob/fc1d41dbb1c03683cf49a44fdb7c9738b8b37d30/src/search.rs#L59-L86

[juchom]

One more thing, this is not a class with logic inside, this is just a DTO. The god object problem does not really apply here.

[ahmednfwela]

I suggest either changing the interface the Dto, which won't be a breaking change if the Dto inherits from the interface.
or adding helper extension methods to cast the interface to the correct concrete types, e.g. .AsSearchResult(),.AsPaginatedSearchResult()

[juchom]

The method should map to only one object without inheritance.

This object is matching the spec of the api, what happened next is pure mapping and the responsability of the end user.

If you think that .AsSearchResult() and .AsPaginatedSearchResult() mapping helpers may be valuable for the developers, you could add them to the package.

[massijay]

I think it's better how the class is implemented in rust (actually it's called with the plural name, SearchResults).
I'm trying to figure out what's the best way to implement the ranking score field (#458) and I think it's better to have the Hit as a generic class (like the SearchResult class in the rust implementation)