meilisearch / meilisearch-go

Golang wrapper for the Meilisearch API
https://www.meilisearch.com
MIT License
542 stars 86 forks source link

Search response unmarshal issues #436

Closed candiduslynx closed 1 month ago

candiduslynx commented 1 year ago

Description

There are several issues with SearchResponse unmarshaling:

  1. These lines are always defaulting to in.Interface() path per v55 being nil interface
  2. There's no way to properly unmarshal uint64 value (in.Interface() -> in.Float64() that loses precision)

Expected behavior

  1. SearchResponse.Hits unmarshaling should be addressed
  2. It should be possible to unmarshal even the largest uint64 value (math.MaxUint64)

Current behavior

  1. SearchResponse.Hits always goes with in.Interface() path
  2. math.MaxUint64 value will lose precision per unmarshaling as float64

Screenshots or Logs

Stored value: uint64(18446744073709551615) Scanned value: float64(9223372036854775808)

Environment (please complete the following information):

brunoocasali commented 1 year ago

Hi @candiduslynx!

The repo's maintainer, @alallema is on holiday. As soon as they get back, they will review your code!

Thanks for the issue!

alallema commented 1 year ago

Hi @candiduslynx, Thanks for raising this issue. I will check that as soon as possible.

alallema commented 1 year ago

Sorry for the delay, It's the week of the engine release, so I'll take a look as soon as possible. By the way it seems to be related to this issue #406

alallema commented 1 year ago

Hello @candiduslynx, I looked closer at the problem of transforming uint64 into float64. According to the documentation, this is the default behavior: json.Unmarshal uses floats when unmarshalling numbers from an []interface{}. I don't see how to change this, given that for greater freedom we need to leave an interface for returning documents and that we can't change the library's basic behavior. My solution for this specific case is to use the SearchRaw and unmarshal you specific struct:

type SearchResponseCustomDoc struct {
    Hits               []CustomDoc `json:"hits"`
    EstimatedTotalHits int64       `json:"estimatedTotalHits,omitempty"`
    Offset             int64       `json:"offset,omitempty"`
    Limit              int64       `json:"limit,omitempty"`
        ...
}

type CustomDoc struct {
    ID           string   `json:"id"`
    Title        string   `json:"title"`
    CustomNumber uint64   `json:"custom_number"`
}

searchResRaw, err := client.Index("customDoc").SearchRaw("", &meilisearch.SearchRequest{})
if err != nil {
    fmt.Println(err)
    os.Exit(1)
}

For your first point:

These lines are always defaulting to in.Interface() path per v55 being nil interface SearchResponse.Hits unmarshaling should be addressed

I'm not sure I understand what you meant exactly, that Hits should be declared as a pointer :

type SearchResponse struct {
    Hits               []interface{} `json:"hits"`
        ...
}
candiduslynx commented 1 year ago

Hi @alallema

json.Unmarshal uses floats

You use easyjson instead of standard encoding/json though.

to use the SearchRaw and unmarshal you specific struct

This might be actually an interesting thing to pursue, I'll take a look.

These lines are always defaulting to in.Interface() path per v55 being nil interface

You use easyjson lib, an in those lines you have the following:

var v55 interface{}
if m, ok := v55.(easyjson.Unmarshaler); ok { // THIS IS AN ISSUE as v55 was just defined and is, in fact, nil
    m.UnmarshalEasyJSON(in)
} else if m, ok := v55.(json.Unmarshaler); ok { // THIS IS AN ISSUE as v55 was just defined and is, in fact, nil
    _ = m.UnmarshalJSON(in.Raw())
} else {
    v55 = in.Interface() // End up here ALWAYS
}

SearchResponse.Hits unmarshaling should be addressed

It just corresponds to what is expected here, and in this case it's v55 discrepancy fixed:

alallema commented 1 year ago

You use easyjson instead of standard encoding/json though.

Yes, indeed, but it should be utterly compatible with the json library. You'd think we could handle the problem differently if we didn't use easyjson?

It just corresponds to what is expected here, and in this case it's v55 discrepancy fixed:

  • either remove the type casting there
  • or assign something to v55 prior
  • or check not the v55 type but something else

Thanks for the details, but this file you're talking about is auto-generated by easyjson. I'll have a look in their documentation to see if there are any options or ways to fix it. If you have a suggestion for solving the problem, I'd love to hear it.

It's not always easy to manage typing for all cases, and some of the details in this SDK are not necessarily adequate. If you have any other feedback, please don't hesitate to contact us.

Thanks 😊

sam-ulrich1 commented 7 months ago

Still a problem

candiduslynx commented 7 months ago

@alallema

I don't see how to change this, given

I think one of the things to be used here is actually something like https://pkg.go.dev/encoding/json#Decoder.UseNumber

Or https://github.com/tcolar/easyjson/blob/master/opt/gotemplate_Uint64.go

candiduslynx commented 1 month ago

@Ja7ad do you mind adding the PR with the fix to the issue? Or was it intended to be closed as won't fix?

Ja7ad commented 1 month ago

@Ja7ad do you mind adding the PR with the fix to the issue? Or was it intended to be closed as won't fix?

This issue is related to EasyJSON, and we currently don't have plans to fix it after 1 year.

Using easyjson can improve performance in marshalling and unmarshalling, but it has limitations, especially when dealing with complex types like interface{}. Since Meilisearch returns search results as Hits with an unknown structure, the flexibility of the built-in encoding/json package might be more suitable for handling dynamic data. It offers more robust error handling, better debugging, and broader community support, albeit with slightly reduced performance.

Switching to the built-in package could make the SDK more maintainable and reliable.

I think we need instead use easyjson we use built-in json and we can make custom marshaller for SearchRespone and fix Hits issue.

Interface so bad for this I think better use *[]map[string]interface{} for dynamic structure.

This is a custom unmarshaller.

type SearchResponse struct {
    Hits *[]map[string]interface{} `json:"hits"`
    // other fields...
}

func (sr *SearchResponse) UnmarshalJSON(data []byte) error {
    type Alias SearchResponse
    alias := &Alias{}
    if err := json.Unmarshal(data, alias); err != nil {
        return err
    }

    var hits []map[string]interface{}
    if err := json.Unmarshal(data, &hits); err != nil {
        return err
    }

    sr.Hits = &hits

    *sr = SearchResponse(*alias)

    return nil
}

After completing the high-priority tasks for v1.10.0 and v1.11.0, I will fix this problem, Kindly include more details about the exception in the new issue.

Ja7ad commented 1 month ago

@candiduslynx Please follow issue #579

candiduslynx commented 1 month ago

cc @erezrokah