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

OpenAPI Spec file is invalid -- IntOrString type is a list #4817

Closed
dcherman opened this issue Jan 1, 2021 · 9 comments · Fixed by #4831
Closed

OpenAPI Spec file is invalid -- IntOrString type is a list #4817

dcherman opened this issue Jan 1, 2021 · 9 comments · Fixed by #4831
Labels
area/api Argo Server API type/bug

Comments

@dcherman
Copy link
Member

dcherman commented Jan 1, 2021

Per the OpenAPI Spec, the type field must not be a list as it only takes a single value. The correct way to specify multiple types is using oneOf and anyOf.

https://github.com/argoproj/argo/blob/4eaae251d9147b53604481237732bff36a16a91e/api/openapi-spec/swagger.json#L7994-L7997

This was found when I was attempting to generate a jsonnet lib from the OpenAPI spec.

@kennytrytek
Copy link
Contributor

I think this can be closed since the project validates against Swagger 2.0 and using oneOf in the JSON schema was added in v3.1.0:
OAI/OpenAPI-Specification#1026

I was initially going to open a PR to fix this by changing this line in kubeifyswagger.go to:

definitions["io.k8s.apimachinery.pkg.util.intstr.IntOrString"] = obj{"oneOf": array{ obj{"type": "string"}, obj{"type": "integer"}}}

But that results in this make codegen failure:

The swagger spec at "api/openapi-spec/swagger.json" is invalid against swagger specification 2.0.
See errors below:
- definitions.io.k8s.apimachinery.pkg.util.intstr.IntOrString.oneOf in body is a forbidden property

Makefile:534: *** [api/openapi-spec/swagger.json] error 1

#0  api/openapi-spec/swagger.json at /Users/kennytrytek/git/argo/Makefile:532
#1  codegen at /Users/kennytrytek/git/argo/Makefile:259

Related reading:
OAI/OpenAPI-Specification#525
zircote/swagger-php#273
zircote/swagger-php#396
OAI/OpenAPI-Specification#333
OAI/OpenAPI-Specification#624

@alexec
Copy link
Contributor

alexec commented Jan 2, 2021

I think we should be maximally compatible with the spec. OpenAPI tools tend to be a bit out of date.

@kennytrytek I think you have the correct place to fix this, but maybe it should just be:

definitions["io.k8s.apimachinery.pkg.util.intstr.IntOrString"] =  obj{"type": "string"}

@kennytrytek
Copy link
Contributor

maybe it should just be:

definitions["io.k8s.apimachinery.pkg.util.intstr.IntOrString"] =  obj{"type": "string"}

Wouldn't that break the IntOr part of IntOrString, so any place that uses IntOrString would need to change to support "string" only?

@alexec
Copy link
Contributor

alexec commented Jan 2, 2021

It would be fine. That can be either an int or a string. String can be int, but int cannot be string.

@kennytrytek
Copy link
Contributor

String can be int, but int cannot be string.

The unit tests agree that int cannot be a string:
https://github.com/argoproj/argo/pull/4831/checks?check_run_id=1647895197

Validation failed - daemon-nginx.yaml: Must validate one and only one schema (oneOf)
Invalid type. Expected: string, given: integer

Is the right solution to update the examples? Isn't this a breaking change for anyone using ints in those fields right now?

@alexec
Copy link
Contributor

alexec commented Jan 5, 2021

I'm not sure what the solution here is now. type cannot be an array, so much be removed, oneOf is not allowed, so must be removed. This leaves an empty definition.

@kennytrytek
Copy link
Contributor

I'd suggest that the long-term solution is to use OpenAPI ≥ 3.1.0 since the discrepancy between the spec file and the json was rectified, and the short-term solution is to do nothing unless this out-of-spec swagger.json makes it harder to contribute to the project.

@dcherman
Copy link
Member Author

dcherman commented Jan 5, 2021

FWIW I'm no longer working on the jsonnet lib that was mentioned and found a workaround for the issue if I ever did want to generate it again, so this issue is not interfering with that effort at all.

It looks like kubernetes itself solves this by using string as the type, and a custom format

https://github.com/kubernetes/apimachinery/blob/master/pkg/util/intstr/intstr.go#L128-L132

@kennytrytek
Copy link
Contributor

PR is open to revert back to only support string. IntOrString was previously defined strictly as a string in this commit. It was changed in this pull request to generate the invalid "int or string" spec. While technically closer to the intent of the field name, supporting both types cannot be implemented until Swagger v3.1 is used, as discussed above.

@agilgur5 agilgur5 changed the title OpenAPI Spec file is invalid OpenAPI Spec file is invalid -- type is a list Aug 3, 2024
@agilgur5 agilgur5 added the area/api Argo Server API label Aug 3, 2024
@agilgur5 agilgur5 changed the title OpenAPI Spec file is invalid -- type is a list OpenAPI Spec file is invalid -- IntOrString type is a list Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Argo Server API type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants