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

RFC: [go] define "any-type" schema as an empty interface in generated go code. #888

Closed
wants to merge 2 commits into from
Closed

RFC: [go] define "any-type" schema as an empty interface in generated go code. #888

wants to merge 2 commits into from

Conversation

ashanbrown
Copy link

I could use some advice on this one. The fix for #51 (PR #60) broke our use of the go generator because it now assumes unspecified (i.e. "any-type") schemas will be map[string]interface{} instead of *interface{}. This change tries to restore the empty interface, albeit as interface{} instead of *interface{}. I'm not sure what the intent of using a pointer to an interface was in the original code but maybe that would be preferred . This code does not change the generated petshop code (but maybe petshop should be updated to include an "any-type" schema).

The "any-type" schema is mentioned at https://swagger.io/docs/specification/data-models/data-types/ which claims to be a guide to openapi 3.0, but I don't see it mentioned by name in the "open-api" spec.

Thanks for your help and for keeping this project moving forward.

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 and ./bin/security/{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\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.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

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@wing328
Copy link
Member

wing328 commented Aug 23, 2018

@ashanbrown thanks for the PR. cc the technical committee for review:

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

@ashanbrown ashanbrown changed the title RFC: Define "any-type" schema as an empty interface in generated go code. RFC: [go] define "any-type" schema as an empty interface in generated go code. Aug 23, 2018
@ashanbrown
Copy link
Author

I was also thinking that maybe we should push the problem upward any introduce an "anytype" openAPIType that would default to "object" but could be mapped to something by typeMapping on a per-generator basis.

type = openAPIType;
} else {
// Handle "any type" as an empty interface
if (openAPIType == "object" && p != null && !ModelUtils.isObjectSchema(p) && !ModelUtils.isMapSchema(p)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashanbrown Please use "object".equals(openAPIType) instead

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashanbrown can you please fix it? Here in Arduino we really appreciate to have this PR merged :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @eclipse1985, I've made that change. It wasn't sure that the team was really offering to merge this though because I didn't see further comments from @antihax, @bvwells or @grokify about this or my follow-up comment at #888 (comment). I'd still be happy to have this merged as is but my impression was there might be a larger issue here.

@ashanbrown
Copy link
Author

Looking through the code I noticed that a number of other (not go) generators have a concept of an "any" type. I'm wondering if if we should just always return "any" for all code generators and let the individual generators map the type to the a primitive, as shown in:

master...launchdarkly:ashanbrown/any-primitive

@eclipse1985
Copy link

Hey there! Any update on this? We really need it :)

Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, according to openapi-spec, the current implementation is correct. openapi-spec takes type definitions from JSON schema (with some further restrictions, which don't apply to this case). JSON schema specifies that object is "unordered set of properties mapping a string to an instance" [1].

On the other hand, I see the usecase that is broken right now and would be fixed by merging this PR, so with my go-language-committee member hat on, I'm ok with this PR .

Having said that, we'd probably need to wait for a major release to merge this, because this change would break backwards compatibility after regenerating code. We should rather put this in go-experimental only. @wing328 do you agree with the above? Thanks!

[1] https://tools.ietf.org/html/draft-handrews-json-schema-02#section-4.2.1

type = openAPIType;
} else {
// Handle "any type" as an empty interface
if (openAPIType.equals("object") && p != null && !ModelUtils.isObjectSchema(p) && !ModelUtils.isMapSchema(p)) {
Copy link
Contributor

@zippolyte zippolyte Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it seems this would only work with the following suggestion.

Suggested change
if (openAPIType.equals("object") && p != null && !ModelUtils.isObjectSchema(p) && !ModelUtils.isMapSchema(p)) {
if (openAPIType.equals("object") && p != null && ModelUtils.isObjectSchema(p) && !ModelUtils.isMapSchema(p)) {

Indeed, a schema like the following will be an ObjectSchema:

AnyType:
  type: object

Otherwise, I agree this should probably be added as part of go-experimental and released in a major

@ashanbrown
Copy link
Author

@zippolyte @bkabrda I don't have much to add on this as I haven't been working with openapi a lot lately. I don't know if you also looked at the alternate proposal at #3878 by @mattiabertorello. I suspect he has a deeper (and more up-to-date) understanding of how this code works than I do. Any change that allows specification of an arbitrary type is would be fine by me. I don't completely follow the change suggested by @zippolyte and would be reluctant to implement it myself without a corresponding test for the behavior we are expecting.

@wing328
Copy link
Member

wing328 commented Jun 16, 2020

@wing328 wing328 closed this Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants