make _formatted value user friendly :)

[ahmednfwela]

Pull Request

Related issue

Fixes #315

What does this PR do?

• Introduces a new type IFormatContainer<TOriginal, TFormatted> coupled with a json converter IFormatContainerJsonConverter<TOriginal, TFormatted> and a IFormatContainerJsonConverterFactory
• Introduces a new SearchAsync overload

  public async Task<ISearchable<IFormatContainer<T, TFormatted>>> SearchAsync<T, TFormatted>(string query,
            SearchQuery searchAttributes = default, CancellationToken cancellationToken = default)

  to support _formatted documents.
• Modified tests to remove the ugly looking FormattedMovie in favor of the new API
• No breaking changes :)

Usage is simple:

• If the model has same signature as the "_formatted" model: use SearchAsync<Model, Model>
• If not, use SearchAsync<Model, FormattedModel>

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

cc @alallema

[juchom]

Hello @ahmednfwela,

Thanks for this PR and this idea to solve the issue.

The main question we have here, is to consider if the issue is coming from the client or from the server response.

My personal opinion, is that the problem is coming from the server, the response is shaped like this thanks to a flattening operation that only exists in rust. In other word, the serialisation of the real object is not following standard json.

It all depends on how Meilisearch team will consider this issue. If they consider this response ok, then the issue is on our side and we will have to do some akward things here. If they consider that using serd's flatten option is not ideal, then the fix will be very natural here.

[ahmednfwela]

I think the flattening is ok here, since following the principle of least surprise, if a user who is used to a list of

{"id": 1, "name": "Hello world"}

suddenly starts seeing a list of

{"original":{ "id": 1, "name": "Hello world"}, "formatted":{ "id": 1, "name": "Hello world"}}

when they request highlighting, it would throw them off.

[juchom]

For me, I think that it's really weird to ask a developer to chose between two functions for searching with different return types.

public async Task<ISearchable<T>> SearchAsync<T>(string query,
            SearchQuery searchAttributes = default, CancellationToken cancellationToken = default)

and

public async Task<ISearchable<IFormatContainer<T, TFormatted>>> SearchAsync<T, TFormatted>(string query,
            SearchQuery searchAttributes = default, CancellationToken cancellationToken = default)

The response should always be the same and the formatted property should always exists as a nullable property.

Otherwise you would have to duplicate some code in your ui depending if you want to see highlighting or not.

The other problem with flattening here, is name collision, what if your model has a _formatted property what will be the value of it ?

[ahmednfwela]

the second method is just a sugar syntax for the first one actually, because you can just do SearchAsync<IFormatContainer<T,TFormatted>>(...)

the developer can keep using IFormatContainer whether _formatted exists or not, but if they are not going to use highlighting EVER and not willing to keep repeating .Original on every property access, I think it's better to use the first overload.

also for name collisions, I think it's normal, as long as it's well-documented it becomes the user's problem to avoid using preserved fields

[juchom]

the developer can keep using IFormatContainer whether _formatted exists or not, but if they are not going to use highlighting EVER and not willing to keep repeating .Original on every property access, I think it's better to use the first overload.

This actual could be solve with this

var item = response.Original;

also for name collisions, I think it's normal, as long as it's well-documented it becomes the user's problem to avoid using preserved fields

Not my opinion, this issue can be avoided without this transformation. Users who are evaluating the option Meilisearch are not going to break what they already have just to make it compatible with it.

I'm sure this is a very low percentage and Meilisearch will have to evaluate what is more interesting for them. This is not only technical, it is now a business decision too.

[ahmednfwela]

it isn't all that rare for databases to enforce field naming rules, e.g. in mongodb

also users don't break what they have to transform to meili, since meili isn't supposed to be a source of truth, I would like to think of meili as a caching layer (like redis) with improved searching capabilities.

I for instance have a c# server talking to a mongodb instance and syncing data with a meilisearch instance, where my mongodb objects have PascalCase field names and my meilisearch instance has camelCase.
I also transform c# datetimes into unix timestamps for before date/after date filters.

what I am suggesting is keep everything as is server side, and let clients figure it out

[juchom]

what I am suggesting is keep everything as is server side, and let clients figure it out

I'm ok with that if this is the decision of Meilisearch team.

What I was more saying is that before merging this PR, we should wait to know what the core team wants to do with this issue.

This is impacting all strongly typed languages. It's not an easy decision to take for them.

[bidoubiwa]

Hey @ahmednfwela!

Just to let you know @alallema is on Holiday and will review this PR once they come back :)

[ahmednfwela]

merged with main and solved conflicts @alallema

[alallema]

Thank you so much @ahmednfwela, for this PR ❤️ and so sorry for the delay ...
I'll make a few comments, but shouldn't we change the samples in the code-sample file according to this new PR?
Also I didn't have the right on this PR to fix the conflicts I let you do it 😃

[alallema]

Why is this method asynchronous if there is no await inside?

[ahmednfwela]

I guess it's just a habit of making everything async 😄

[ahmednfwela]

@alallema Thanks for your review!

I think after the recent changes in meilisearch, it's better if we take an approach similar to this PR meilisearch/meilisearch-dart#350

where we make a comprehensive MeiliDocumentContainer<T> class, which wraps around the document and strongly types all the meilisearch-related fields, without making any assumptions on the returned type.

what do you think ?

[alallema]

@ahmednfwela, I more than agree. But I think it's up to @brunoocasali to take the decision

[brunoocasali]

where we make a comprehensive MeiliDocumentContainer class, which wraps around the document and strongly types all the meilisearch-related fields, without making any assumptions on the returned type.

But I think that triggers the same issue, no? Because it may not be possible to do it...

[brunoocasali]

bors try

[ahmednfwela]

@brunoocasali I am currently working on a refactor for this PR

[brunoocasali]

@brunoocasali I am currently working on a refactor for this PR

Thank you very much @ahmednfwela ;)

[curquiza]

@ahmednfwela, thank you again for this PR. DO you think we should continue to work on it?

[ahmednfwela]

@curquiza Hi, and sorry for the long delay, but yes, I am currently updating this PR.

[danFbach]

@ahmednfwela do you need any assistance with this?
I like your initial PR, but I had an another idea which may be simpler while providing the same functionality.

create an abstact class

    /// <summary>
    /// Inheritable class for retrieving formatted results.
    /// </summary>
    /// <typeparam name="T"></typeparam>
    public abstract class MeilisearchDocumentContainer<T>
    {
        /// <summary>
        /// Gets or sets the _formatted field.
        /// </summary>
        [JsonPropertyName("_formatted")]
        public T _Formatted { get; set; }
    }

Then if the user sets the "AttributesToHighlight" and are expecting _formatted to be returned, they just define their class as inheriting from MeilisearchDocumentContainer.

This method passes all existing tests without modification (other than the Movie class inheriting from MeilisearchDocumentContainer and replacing the FormattedMovie with Movie that inherits from MeilisearchDocumentContainer) and gives users the ability to easily use the _formatted feature and would be a non-breaking change.

Let me know your thoughts!

[ahmednfwela]

hi @danFbach , my main problem with inheritance, is that we are restricting the user to use our base class, so if they have their own base class for dtos, they will be unusable

you are free to pick up the work here though, as I am currently extremely busy, there is a similar implementation in meilisearch-dart repo if you want it for reference