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

Search response unmarshal issues #436

Closed
candiduslynx opened this issue May 16, 2023 · 12 comments
Closed

Search response unmarshal issues #436

candiduslynx opened this issue May 16, 2023 · 12 comments
Labels
bug Something isn't working

Comments

@candiduslynx
Copy link
Contributor

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):

  • OS: Linux
  • Meilisearch version: v1.1.0
  • meilisearch-go version: v0.24.0
@brunoocasali
Copy link
Member

brunoocasali commented May 16, 2023

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!

kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this issue May 18, 2023
@alallema alallema added the bug Something isn't working label May 22, 2023
@alallema
Copy link
Contributor

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

@alallema
Copy link
Contributor

alallema commented Jun 5, 2023

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 alallema added the needs investigation Needs to take time to understand the issue. label Jun 6, 2023
@alallema
Copy link
Contributor

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
Copy link
Contributor Author

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:

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

@alallema
Copy link
Contributor

alallema commented Jun 14, 2023

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 😊

@alallema alallema removed the needs investigation Needs to take time to understand the issue. label Sep 7, 2023
@sam-ulrich1
Copy link

Still a problem

@candiduslynx
Copy link
Contributor Author

@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

@Ja7ad Ja7ad closed this as completed Sep 23, 2024
@candiduslynx
Copy link
Contributor Author

@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
Copy link
Collaborator

Ja7ad commented Sep 23, 2024

@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
Copy link
Collaborator

Ja7ad commented Oct 2, 2024

@candiduslynx Please follow issue #579

@candiduslynx
Copy link
Contributor Author

cc @erezrokah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants