Do not check status code for default response

[thiagoarrais]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
• Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fixes #3321 by not checking HTTP status when generating code for a default response.

Reviewers, please check review comments.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09)

[thiagoarrais]

This counts on the fact that the wildcard response will be the last one. My (limited) testing confirmed this, but will that always be the case?

[antihax]

Ideally this should catch any error code so the error model is returned if one is specified. It may need to catch the default/wildcard in these cases.

If i remember correct, this should be an interface so it could be possible to make this completely generic without specifically checking each code?

[thiagoarrais]

Ideally this should catch any error code so the error model is returned if one is specified. It may need to catch the default/wildcard in these cases.

My approach here was supressing the status code check for the default case so that it handles any error code. But that may overcatch if the wildcard response isn't the last one. Hence my original question.

If i remember correct, this should be an interface so it could be possible to make this completely generic without specifically checking each code?

Sorry, I don't understand. Are you suggesting removing that check completely and just trying to unmarshall the error model based on datatype?

[antihax]

Can you regenerate the test client?

[thiagoarrais]

Done in 942f65f. I was saving that for when the PR got closer to finishing.

[thiagoarrais]

I'm getting this double return on the generated code:

// ...
	err = a.client.decode(&v, localVarBody, localVarHttpResponse.Header.Get("Content-Type"))
	if err != nil {
		newErr.error = err.Error()
		return localVarReturnValue, localVarHttpResponse, newErr
	}
	newErr.model = v
	return localVarReturnValue, localVarHttpResponse, newErr

	return localVarReturnValue, localVarHttpResponse, newErr
}

I'd like to avoid it, but my mustache-fu is weak. Any ideas?

[thiagoarrais]

I've rebased this to latest master and ported the change to go-experimental. I'll start seeking a solution to my previous question. Any blockers here besides that?

[wing328]

cc @bkabrda for review as the change targets go-experimental

[bkabrda]

Hmm, it's weird that this is something that happens only in the api_default.go and not the other api_*.go. I'm wondering whether it might be a general problem in openapi-generator that occurs for certain responses. I'll try to dig in a bit and see if I find out something.

[bkabrda]

So my understanding of the issue is this:

• Any wildcard response (that includes "4XX"-like responses and also "default") have code == 0.
• Wildcard responses can be used alongside of other specific responses.
• It's true that the go generated code won't work for any wildcard responses.

However, I think the solution presented here is not entirely correct. The problem, I think, is that if there was e.g. also a 400 response defined, it could theoretically be listed after the default response in the responses array in the Java code. That would mean that not adding the if clause for wildcards would actually catch a 400 response instead of the more specific 400 response handler (which would have it's code generated after the default handler).

I guess the correct solution is to both:

1. Do what this patch is doing.
2. Make sure the Java code sorts the responses in order from the more specific ones to the less specific ones. E.g. for an endpoint with "default", "4XX" and "400" responses defined, they should be listed in order "400", "4XX", "default".

If the above problem is also in templates for other languages, having the responses sorted would also help all of them.

Does this sound reasonable?

[bkabrda]

Minor clarification of the above comment: it seems that the wildcard response numbers such as "4XX" actually produce invalid code (at least for Go), as they're treated as numbers, so there will be literal 4XX put in the generated code. I think in addition to what I wrote above, we'll also need to mark these as wildcards and set their code to 0.

@thiagoarrais do you want to work on the points that I mentioned? I think I could find some time within ~1 week to get these fixed if you can't/don't want to work on them.

[thiagoarrais]

Make sure the Java code sorts the responses in order from the more specific ones to the less specific ones. E.g. for an endpoint with "default", "4XX" and "400" responses defined, they should be listed in order "400", "4XX", "default".

I can work on that, but I haven't looked into the Java code yet. Can you point me into the right direction?

Re: treating numbered wildcard responses ("4xx"): I'd prefer to keep this PR focused on literal default responses ("default").

[bkabrda]

@thiagoarrais so you'll need to modify org.openapitools.codegen.DefaultCodegen. In the method fromResponse, you'll have to put r.code = 0 for the 4XX (and similar) wildcards - you can find the list of all possible wildcards at [1].

To modify the order of responses, you'll need to do sorting on op.responses in the fromOperation method (also on DefaultCodegen).

I'm somewhat new to openapi-generator myself, but I think this would be all that you need to do to get this done.

[1] https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#patterned-fields-1

[thiagoarrais]

Once CI finishes this is done by me. The other wildcards (1xx, 2xx, and so on) needed a little more investigation and were left for a later PR. Please let me know if you need anything else.

[bkabrda]

Right, so even though we haven't reached a complete solution yet, we certainly improved the situation here, so 👍 for me to merge it. @thiagoarrais will you be interested in taking this further in a followup PR? I can work on it if you're not interested (I'd like to solve this now to get a complete solution to the problem).

@wing328 feel free to review, I have no further comments here.

[thiagoarrais]

Can't work on that right now, but maybe in a near future. Maybe we can open an issue as a placeholder for whomever finds the time first?

[bkabrda]

Once this is merged, I'll open a followup issue and work on it, time permitting.

[thiagoarrais]

Found some potential issues with this. Please avoid merging for the time being.

By the way: can't figure how to mark a PR as draft after it is already opened...

[thiagoarrais]

My problem seems to raise in OpenAPI 3 specs. Still digging, but it seems like the sorting code in DefaultCodeGen isn't getting run or isn't effective. Any ideas?

[thiagoarrais]

OK. This is ready. I had switched the sorting comparation around.

[thiagoarrais]

Can't tell if the Travis failure is related to the PR. Any pointers?

[thiagoarrais]

@wing328: anything else needed for going forward with this?

[wing328]

@thiagoarrais I notice that you've updated the test to replace 2xx with 4xx status code.

Does it mean the original "201" status code test fail?

[thiagoarrais]

No. 422 just seemed more realistic. 201 is a success and more likely to be equated to the default case.

[wing328]

Changes in DefaultCodegen

cc @OpenAPITools/generator-core-team

[thiagoarrais]

Squashed and rebased. Anything else needed for merging?

[thiagoarrais]

Folks, anything else that needs to be addressed here?

[wing328]

@thiagoarrais Let me review by tomorrow ...

[wing328]

Sorry for the delay. Please resolve the conflicts and I'll merge after that 🙏

[thiagoarrais]

Hey, @wing328, no need to apologize. We all understand that.

Anyway... resolved conflicts and rebased. Should be good to go.

[thiagoarrais]

Ran update as requested by CircleCI. Regarding the Travis failure, I'm not sure it is related to this PR, since a similar build already ran successfully for my fork.

[wing328]

CircleCI issue not related to this PR. Tests passed locally:

=== RUN   TestUpdateUser
--- PASS: TestUpdateUser (0.33s)
=== RUN   TestDeleteUser
--- PASS: TestDeleteUser (0.16s)
PASS
ok  	_/private/var/tmp/openapi-generator/samples/client/petstore/go	7.199s
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:20 min
[INFO] Finished at: 2019-10-15T01:28:26+08:00
[INFO] ------------------------------------------------------------------------

[wing328]

@thiagoarrais PR merged into master. Thanks for your contribution.

[wing328]

@thiagoarrais thanks for the PR, which has been included in v4.2.0 release: https://twitter.com/oas_generator/status/1189824932345069569

[thiagoarrais]

Awesome! It's been a pleasure!