[FEATURE] HTTP response as second return value of functions of the opensearchapi package

[ste93cry]

Is your feature request related to a problem?

In a project I'm working on, I have some integration tests that are testing some code that relies on the functions of the opensearchapi package. However, when it's time to assert that the output is equal to the expectations, it's not possible to write a simple require.Equal(t, wantResp, resp) because the response object returned by the client includes the response *opensearch.Response field, which is hard to predict.

What solution would you like?

The solution I propose is to return the HTTP response as second return value, instead of inside the API response object. In this way, SomeResp becomes somewhat predictable, as it would no longer contain a field whose value is unknown until the request is actually made. In this way, code like the one below is possible without much effort:

resp, err := s.opensearchClient.SomeFunc(context.Background(), SomeReq{
	// Params
})

want := &elastic_types.SomeResp{
	// Expected values
}

require.NoError(t, err)
require.Equal(t, want, resp)

Roughly, the changes would be these ones:

type SomeResp struct {
-	response *opensearch.Response
}

-func (c *Client) SomeFunc(ctx context.Context, req SomeReq) (*SomeResp, error) {
+func (c *Client) SomeFunc(ctx context.Context, req SomeReq) (*SomeResp, *opensearch.Response, error) {
	var (
		data SomeResp
-		err  error
	)

-	if data.response, err = c.do(ctx, req, &data); err != nil {
+	resp, err := c.do(ctx, req, &data)
+	if err != nil {
-		return &data, err
+		return nil, resp, err
+	}

-	return &data, nil
+	return &data, resp, nil
}

What alternatives have you considered?

An alternative solution might be to compare each field of the response object separately. However, this doesn't scale well when there are a lot of fields and makes the code longer and harder to read and maintain.

Do you have any additional context?

As this is a breaking change, the downside is that it might take quite some time to be released. I would also be open to contribute this change myself, if accepted.

[Jakob3xD]

We made the opensearch.Response "hidden" behind the Inspect() as most users don't need this response and changing this because of some test to look better ... idk.

We are already trying to find missing fields in our tests with json compare. So you might consider using some sort of json compare.

opensearch-go/internal/test/helper.go

Line 123 in bd5d233

func CompareRawJSONwithParsedJSON(t *testing.T, resp any, rawResp *opensearch.Response) {

[ste93cry]

The problem is not that the field is "hidden" behind an interface, but rather that it is impossible to compare the two structs without resorting to hacks like the one you are mentioning. As you said, a user is often not interested in the raw response, so writing a simple assertion that compares the returned response should work without much hassle. Using an helper like the one you linked works, but it's just a way to solve the problem that the single responsibility principle is broken in this context. I took the idea of returning the response as 2nd value from the Datadog SDK, and I find it quite clear and nice. Returning multiple values is idiomatic in Go, and it makes explicit what is being returned by just looking at the function signature.

[dblock]

@ste93cry want to give a PR a shot so we can look at it?

Generally I like explicit returns personally too, but I am concerned about backwards compatibility as well.

[Jakob3xD]

it's not possible to write a simple require.Equal(t, wantResp, resp) because the response object returned by the client includes the response *opensearch.Response field, which is hard to predict.

Have you tried using EqualExportedValues()? Looks like what you want.
https://pkg.go.dev/github.com/stretchr/[email protected]/require#EqualExportedValues

[ste93cry]

I tested your solution and it works, definitely. However, my 2 cents, I still think there is no real reason to couple the HTTP response with an object that represents the deserialized data. If backwards compatibility is the only concern here, I wouldn't really bother too much, as fixing it is quite easy on a practical level.

[Jakob3xD]

My initial idea behind this was following. I did not want to omit the http response all the time as it is not relevant in most cases. So why return something you barely need.
This was the discussion about it.
#65 (comment)

[ste93cry]

as it is not relevant in most cases
So why return something you barely need.

I agree on this to a certain degree: I found some cases where the definition of the SDK types are not complete, meaning that not all fields are available. In those cases, having access to the response is definitely useful. Another common use case for the response is debugging. Note I'm not saying that the way to access the response matters here, but they are just two examples of why in some cases the HTTP response is relevant.

This part of a comment in that discussion exactly explains why I think that the current solution is suboptimal:

This is awkward because it has hidden magic

[dblock]

I'll try to help here to break a tie ;)

@Jakob3xD, absent of any existing code, if we were writing this library from scratch, do you agree that it's better to return the underlying response explicitly like suggested here? If yes, the pros of the change is that it makes access to the underlying response explicit, and the cons is that it's a breaking change.

If we can all agree to this, then we need to decide when we take the change.

My 0.02c is that we should merge a breaking change like this when we want to make other major breaking changes. As an example, @nhtruong rewrote the opensearch-js client in TypeScript and rolled out a new code generator from API spec in opensearch-project/opensearch-js#803, so together that represented a significant enough improvement to make non-backwards-compatible API changes.

[nhtruong]

@dblock small correction: I rewrote the client to consolidate some features but the client is still basically common JS. Only the generator is in TS. Your point still stands though. You will most likely introduce breaking changes when API is switched to generated code, so might as well take advantage of the opportunity to fix other glaring issues that also require a major version bump. We had discussion on what to be included in the rewrite here for reference.

[ste93cry]

when we want to make other major breaking changes

A bit off-topic, but if we want to talk about another major breaking change that would level up the SDK, we should consider providing users with typed request objects. Right now, when using the opensearchapi package, you're left to look at the documentation of each API, understand which fields it needs, write some objects representing the request body and then serialize them to JSON because that's what the SDK expects. To be honest, what's the point of using the SDK then? If we consider the improvements that can be made to this package, most will be breaking changes in a way or another imho.

[dblock]

A bit off-topic, but if we want to talk about another major breaking change that would level up the SDK, we should consider providing users with typed request objects

100%. Someone (you?) should take on #284