V3: Add initial version of new OpenAPI generator

[pierre-emmanuelJ]

Description

Aim of this change is to directly generate the new go bindings from the OpenAPI spec with our homemade generator. This let us able to solve all the blockers we have with the current version and the https://github.com/deepmap/oapi-codegen generator.

The Goal of this change, is to generate code close as possible to the Spec, with a ready to use, nice user experience.

[pierre-emmanuelJ]

I don't have that much to comment on this PR, the generator code looks to work and produce a result. But I'm not sure how to evaluate the generated code.

Especially given that the XIP is still in WIP and thus doesn't clearly define how the generate code should be used.

I still have some things to point out though:

• Having the context be embedded inside the client object and not passed directly to the method/API call is at odds with genera go advice: "Contexts should not be stored inside a struct type, but instead passed to each function that needs it" @aureliar8

Yes context should be added on each method, I agree 👍

• The current error handling and reporting is lacking, I think a few error wrapping could go a long way in making errors understandable for the customer, but this can probably be go in an other PR as it'll not introduce breaking changes

You mean the generated code error handling for customer, or the generator error handling? On the generator side, maybe more error handling is needed, yes, on the operations method calls for the moment there is a middleware, to trigger better error from http...etc: https://github.com/exoscale/egoscale/blob/v3-experimental-generator/v3/api/middleware.go

Those are registered at client creation.

I'm open on ideas to improve it also...etc and maybe more wrapped error as you mentioned.

[aureliar8]

You mean the generated code error handling for customer, or the generator error handling?

I mean the error handling of generated code that the customer will use.

Let's use this generated method as an example

// List IAM Access Keys
func (c ClientAPI) ListAccessKeys() (*ListAccessKeysResponse, error) {
    path := "/access-key"

    u, err := url.Parse(c.serverURL + path)
    if err != nil {
        return nil, err
    }

    request, err := http.NewRequestWithContext(c.ctx, "GET", u.String(), nil)
    if err != nil {
        return nil, err
    }
    request = request.WithContext(c.ctx)

    if err := registerRequestMiddlewares(&c, c.ctx, request); err != nil {
        return nil, err
    }

    resp, err := c.httpClient.Do(request)
    if err != nil {
        return nil, err
    }

    if resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusIMUsed {
        return nil, fmt.Errorf("request returned %d code", resp.StatusCode)
    }

    bodyresp := &ListAccessKeysResponse{}
    if err := prepareJsonResponse(resp, bodyresp); err != nil {
        return nil, err
    }

    return bodyresp, nil
}

There are a lot of "naked" return nil, err, I think they could be improved to rather return a wrapped error with a bit more context.

For example

        u, err := url.Parse(c.serverURL + path)
    if err != nil {
        return nil, fmt.Errorf("ListAccessKeys: generate URL: %w", err)
    }

As an other example ListAccessKeys() can currently return context.DeadlineExceeded, but ti's impossible for the customer to know if it timed out while sending the requests or while reading the very long response body.

---

Also, now that I carefully look at the generated code, there are a few things that seems a bit weird and unnecessary: 1) Calling url.Parse() and http.NewRequestWithContext(): http.NewRequestWithContext() already calls url.Parse() internally, so I think we can just drop that manual call to url.Parse() 2) Calling http.NewRequestWithContext() and then directly request = request.WithContext(c.ctx): the context is already associated to the request

I'm also having some trouble understanding which piece of code should react to non 2XX http codes. Is it the code inside the operation or the ErrorHandlerMiddleware that seems to be always installed ?

[aureliar8]

I'm also a bit confused about how middleware work here:

There seems to be two types of middleware, one used for NewTraceMiddleware and NewAPIErrorHandlerMiddleware where they wrap the http.RoundTrip interface, and one used for security.Intercept that can just modify the request.

Why exactly is there two different ways to do that ? Also having an http.RoundTripper implementation that try to interpret the response is forbiden by the RoundTripper interface

We can probably make a few improvements here by not overriding the internal http.RoundTripper but rather having both request and response interceptors.

[pierre-emmanuelJ]

@aureliar8 I applied your propositions, thanks comments relative to package api is still for discussion, let's come back later on here or in other PR.

[pierre-emmanuelJ]

@aureliar8 @sauterp @kobajagi We are agreed then here to remove the huge generated interface that it was for mocking purposes, we leave now the consumer write its own interface, to generate mocks in its side?

[kobajagi]

@aureliar8 @sauterp @kobajagi We are agreed then here to remove the huge generated interface that it was for mocking purposes, we leave now the consumer write its own interface, to generate mocks in its side?

Yes, we should remove it.

[pierre-emmanuelJ]

Interface generation has been removed.

[sauterp]

Interface generation has been removed.

That happened faster than I could react xD

To me this looks like a misunderstanding @aureliar8 @sauterp @kobajagi

We had the discussion about the mocks in the XIP and let me sum up how I understood the conclusion. In the XIP we decided to remove the mocks, because it would increase the maintainability burden unnecessarily. On the other hand, we agreed that there is value in having Go type interfaces for all public types that are part of the API, including the Client. IMO it's also good Go style to have interfaces as the return types of public functions instead of specific types.

We specified this here: https://github.com/exoscale/doc/pull/2574/files#diff-b5c4ae25a5d68797f2625c210869c73f365e5e6752df845dac512cf381f43dbdR132

So the conclusion as I understood it was, we remove the mocks themselves, but we keep the interfaces. My comments about removing mocks in this PR reflected this.

I'm in favor of keeping the interfaces and removing just the mocks. If the 3 of you disagree, it's not a big deal to me and we can keep it like this. But then we should update the XIP.

[aureliar8]

IMO it's also good Go style to have interfaces as the return types of public functions instead of specific types.

I don't think there is consensus for that in the community and I personally tend to disagree. I'm more in the "return a concrete type and let the user build his own interface" camp.

However I don't know well the domain in which this code will be used, so maybe it's more common there.

I have no strong opinion here and I'm fine with keeping the interface if you think it's better.

[aureliar8]

Actually, I've been thinking about it and I think that exposing the interface would have weird implication in term of maintaining backward compatibility in the future.

Let's say that we expose an interface ApiClient that has a method per route of public API. We expect the customer to directly reference this interface when he stores the exoscale client in some field.
He then implements this interface in his own code either via a mocking lib or manually for his tests.

Now we add a new endpoint in public api, egoscale is regenerated and a new method is added to this interface. Then the customer bumps the version of egoscale: now his code doesn't compile anymore because his mocks don't implement the ApiClient anymore as a new method has been added.

So, unless I'm mistaken, exposing an public interface with the list of all exoscale methods/route would mean that we should do a major version bump for every new route added to public API or break customers' code that rely on this type for their tests.

wdyt ? @sauterp

[kobajagi]

Egoscale XIP should be updated (i'll do that later). Interface would make much more sense if we have mocks available as part of the library. Otherwise interface will be awkward to use on it's own. Only CLI implements (or should implement) all endpoints, tf-provider implements most of them and all other tools use a small portion of API. No need for them to generate mocks for what they don't use (even if just dummy methods).

[sauterp]

@aureliar8 You're absolutely right, that would break consumer mocks. I haven't thought about this before. And predrag is right too, providing the full interface only makes sense then if we also provide the mocks in the package. TIL!

So I'm convinced too, the Client interface is not needed.

You're also right about the interface return types. In our case, having interfaces as the return types in function signatures is not important.

The reason why I thought it matters, was because we did this exploration where we mocked predrags approach to the v3 API. In that approach the Client had methods that return types which yet again have methods of their own. In that scenario you may want to write mocks with methods that can return other mocks. Although some would argue that in this case your testing more than just one 'unit' since you have multiple mocks. In this case it's important to have interface return types, because you're code may not compile. To illustrate this I made this example, hope it makes sense. https://go.dev/play/p/mTJiB00MJas But anyway, this was only relevant to the earlier v3 approach. And just now, I checked all the function signatures in this PR, it looks like all our API route methods only returns types that don't have methods of their own. So consumers won't ever need to mock behaviour of objects returned by the API routes.

So, we're good, thanks for the discussion. We only need to update the XIP then.