Skip to content

Split Response Types by Encoding/Status#231

Open
natsukagami wants to merge 5 commits into
oapi-codegen:mainfrom
curvegrid:response-types
Open

Split Response Types by Encoding/Status#231
natsukagami wants to merge 5 commits into
oapi-codegen:mainfrom
curvegrid:response-types

Conversation

@natsukagami
Copy link
Copy Markdown
Contributor

@natsukagami natsukagami commented Sep 21, 2020 •
edited
Loading

What is the problem?

Currently, the response struct in client-with-responses have all their possible output structures embedded into a single struct. For example, from components.gen.go:

type EnsureEverythingIsReferencedResponse struct {
	Body         []byte
	HTTPResponse *http.Response
	JSON200      *struct {

		// Has additional properties with schema for dictionaries
		Five *AdditionalPropertiesObject5 `json:"five,omitempty"`

		// Has anonymous field which has additional properties
		Four      *AdditionalPropertiesObject4 `json:"four,omitempty"`
		JsonField *ObjectWithJsonField         `json:"jsonField,omitempty"`

		// Has additional properties of type int
		One *AdditionalPropertiesObject1 `json:"one,omitempty"`

		// Allows any additional property
		Three *AdditionalPropertiesObject3 `json:"three,omitempty"`

		// Does not allow additional properties
		Two *AdditionalPropertiesObject2 `json:"two,omitempty"`
	}
	JSONDefault *struct {
		Field SchemaObject `json:"Field"`
	}
}

Notice that there are two possible outputs, JSON200 and JSONDefault, which are anonymous structs.

The anonymous structs make it difficult to perform certain
tasks with it.

Redundant copy-pasting of definition

One such use is passing the struct into a function.

    resp, err := client.EnsureEverythingIsReferenced()
    if err != nil {
        return err
    }
    if resp.JSON200 != nil {
        handleResponse(resp.JSON200) // what is the signature for handleResponse
    }

The handleResponse function has to either have the entire anonymous struct declaration copied to its signature...

func handleResponse(resp *struct {
		Five *AdditionalPropertiesObject5 `json:"five,omitempty"`
		Four      *AdditionalPropertiesObject4 `json:"four,omitempty"`
		JsonField *ObjectWithJsonField         `json:"jsonField,omitempty"`
		One *AdditionalPropertiesObject1 `json:"one,omitempty"`
		Three *AdditionalPropertiesObject3 `json:"three,omitempty"`
		Two *AdditionalPropertiesObject2 `json:"two,omitempty"`
	}) {
	// ...
}

or declare another struct with identical layout, either way takes a redundant copy-paste.

Another example is creating a struct of the same type.
The generated code for the client looks like this
(see diff):

	case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 200:
		var dest struct {

			// Has additional properties with schema for dictionaries
			Five *AdditionalPropertiesObject5 `json:"five,omitempty"`

			// Has anonymous field which has additional properties
			Four      *AdditionalPropertiesObject4 `json:"four,omitempty"`
			JsonField *ObjectWithJsonField         `json:"jsonField,omitempty"`

			// Has additional properties of type int
			One *AdditionalPropertiesObject1 `json:"one,omitempty"`

			// Allows any additional property
			Three *AdditionalPropertiesObject3 `json:"three,omitempty"`

			// Does not allow additional properties
			Two *AdditionalPropertiesObject2 `json:"two,omitempty"`
		}
		if err := json.Unmarshal(bodyBytes, &dest); err != nil {
			return nil, err
		}
		response.JSON200 = &dest

we can see a similar dedundant copy-paste pattern.

Difficult to enforce server-client type equality

Suppose we wanted to enforce the server to return the same type as the client definition.
This is commonly a source of problems, where the server is returning a different structure than the client expects.

This is hard to perform with a anonymous struct, because creating such structs are more difficult and error-prone.

If we wanted to write custom logic for responses (for example, validation of responses, to be used in testing), the embedded structs don't help.

Our approach

In short, we separate all different responses into
independent type declarations.

For example, the above EnsureEverythingIsReferencedResponse will now be generated as

// EnsureEverythingIsReferencedResponseJSON200 represents a possible response for the EnsureEverythingIsReferenced request.
type EnsureEverythingIsReferencedResponseJSON200 struct {

	// Has additional properties with schema for dictionaries
	Five *AdditionalPropertiesObject5 `json:"five,omitempty"`

	// Has anonymous field which has additional properties
	Four      *AdditionalPropertiesObject4 `json:"four,omitempty"`
	JsonField *ObjectWithJsonField         `json:"jsonField,omitempty"`

	// Has additional properties of type int
	One *AdditionalPropertiesObject1 `json:"one,omitempty"`

	// Allows any additional property
	Three *AdditionalPropertiesObject3 `json:"three,omitempty"`

	// Does not allow additional properties
	Two *AdditionalPropertiesObject2 `json:"two,omitempty"`
}

// EnsureEverythingIsReferencedResponseJSONDefault represents a possible response for the EnsureEverythingIsReferenced request.
type EnsureEverythingIsReferencedResponseJSONDefault struct {
	Field SchemaObject `json:"Field"`
}

type EnsureEverythingIsReferencedResponse struct {
	Body         []byte
	HTTPResponse *http.Response
	JSON200      *EnsureEverythingIsReferencedResponseJSON200
	JSONDefault  *EnsureEverythingIsReferencedResponseJSONDefault
}

Note that the response now go through another struct-wrapping.

This might be a breaking change, for example, if the response is just a reference to a schema:

// old style

type FindUsersResponse struct {
    Body         []byte
    HTTPResponse *http.Response
    JSON200      []Users
}

// new style

type FindUsersResponseJSON200 []Users

type FindUsersResponse struct {
    Body         []byte
    HTTPResponse *http.Response
    JSON200      FindPetByIdResponseJSON200
}

In such case, we provide an "escape-hatch" to the old style by enabling the --alias-types command line option (which is defined in oapi-codegen to aggressively alias types). If the option is specified, all new type declarations will instead become type aliases. This means no wrapping whatsoever.

// new style, with --alias-types

type FindUsersResponseJSON200 = []Users

type FindUsersResponse struct {
    Body         []byte
    HTTPResponse *http.Response
    JSON200      FindPetByIdResponseJSON200
}

Status of the PR

The implementation is complete.

This change is Reviewable

@ghost
Copy link
Copy Markdown

ghost commented Sep 22, 2020

Is this change backwards compatible with existing generated code, meaning, if someone re-generates their code, will their existing code break? It seems to me like it should not, unless they've copy/pasted anonymous types into their code.

I've started a v2 branch for changes which break people's code. We might have to put it into v2 if that's the case.

@natsukagami
Copy link
Copy Markdown
Contributor Author

natsukagami commented Sep 22, 2020

It would break code that has responses defined as references to a schema.

For example

type Pet struct { /*...*/ }
type AResponse {
  JSON200 *Pet
}

// becomes
type AResponseJSON200 Pet
type AResponse {
  JSON200 *AResponseJSON200
}

would now break if the user passes AResponse.JSON200 to a function that takes a *Pet.

@natsukagami natsukagami mentioned this pull request Sep 22, 2020
Closed
@dahu33
Copy link
Copy Markdown
Contributor

dahu33 commented May 28, 2021

@deepmap-marcinr would you mind having a second look? With the latest version of this PR, I think @natsukagami found a good compromise between backward compatibility and usability/functionality. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@natsukagami @dahu33