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

Fix no object properties #156

Merged
merged 2 commits into from Nov 28, 2022

Conversation

jcdlbs
Copy link
Contributor

@jcdlbs jcdlbs commented Nov 6, 2022

This pull request fixes an issue encountered using the Kubernetes "Patch" API where Martian complains that the patch body parameter can't have any properties. This is because a patch reference in the body field is defined as follows:

"io.k8s.apimachinery.pkg.apis.meta.v1.Patch": {
"description": "Patch is provided to give a concrete name and type to the Kubernetes PATCH request body.",
"type": "object"
}

This "object" does not define any parameters or additionalParameters and I think (based on my limited reading of the Swagger v2 and JSON schema specs that this means that the 'object' can have any parameters.

I've included a test case to detect this scenario and the code change does not impact any other Martian tests but I'm not yet 100% certain about this and am interested in whether this pull request might be acceptable.

This code solves my specific problem calling :patch-core-v-1-namespaced-secret but is not extensively tested.

@jcdlbs jcdlbs marked this pull request as ready for review November 7, 2022 00:02
@oliyh
Copy link
Owner

oliyh commented Nov 7, 2022

Hello,

From what I can tell additionalProperties defaults to true so technically speaking the existing closed behaviour of martian is wrong, however in the general case I believe this adds value and this is the first time I've heard of an issue with it.

I'm generally in favour of this PR because and object with no declared properties is a bit useless anyway.

I believe for completeness this behaviour should be added to the OpenAPI parser too: https://github.com/oliyh/martian/blob/master/core/src/martian/openapi.cljc#L74 - are you comfortable doing this?

Cheers

@jcdlbs
Copy link
Contributor Author

jcdlbs commented Nov 7, 2022

Yes, I believe so. I'll try to rig up a test case and add handling there too.

BTW - thanks for creating and managing this project!!

@jcdlbs
Copy link
Contributor Author

jcdlbs commented Nov 7, 2022

I've applied the exact same logic to the openapi parser and have added a test case.

Notes...

  • The kubernetes openapi yaml file was created via a tool to translate from Swagger v2 to Openapi V3. I can only assume that it's valid as the tool did not complain and it parsed fine.
  • This has not been tested outside of the existing test coverage in the project. I think it's safe for real world usage but I don't currently have an easy openapi target for testing.
  • Question: Is :additionalProperties handling missing from the OpenAPI parser? It does not seem to match the logic in the Swagger parser. I don't have time to look into this but I wanted to mention it just in case it's an oversight.

@oliyh
Copy link
Owner

oliyh commented Nov 8, 2022

Hi,

Yes it looks like additionalProperties wasn't handled in the OpenAPI parsing, thanks for bringing that across too. This looks good, it just needs rebasing I think to fix the conflict and I'm happy to merge.

Thanks!

Allow for any object parameters if neither parameters nor
additionalParameters are supplied.

The Kubernetes API, for example, has the following specified in
Swagger V2 for the Patch reference:

    "io.k8s.apimachinery.pkg.apis.meta.v1.Patch": {
      "description": "Patch is provided to give a concrete name and type to the Kubernetes PATCH request body.",
      "type": "object"
    }

Without this commit there is no way to send a Patch via Martian to the
Kubernetes API.
@jcdlbs
Copy link
Contributor Author

jcdlbs commented Nov 12, 2022

Rebased

@oliyh oliyh merged commit 9bd47c7 into oliyh:master Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants