Fix no object properties

oliyh / martian

The HTTP abstraction library for Clojure/script, supporting OpenAPI, Swagger, Schema, re-frame and more

MIT License

529 stars 44 forks source link

Fix no object properties #156

Closed jcdlbs closed 1 year ago

jcdlbs commented 1 year ago

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.

oliyh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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!

jcdlbs commented 1 year ago

Rebased