fix "nullable" decoding

[brandonbloom]

The "nullable" keyword is inconsistently handled. Decoding logic exists, but when the "type" keyword is recognized, the [..., "null"] array pattern takes precedence. This change fixes that, so both work.

[mattpolzin]

Does the OpenAPIKit30 module handle the nullable keyword correctly for you? The OpenAPIKit module decodes OAS 3.1 documents and the nullable keyword was removed from OAS in version 3.1 (as I recall). This means an OAS 3.0 document with “null” in its type array is malformed (but correct for OAS 3.1) and similarly an OAS 3.1 document with a nullable keyword specified is malformed (but correct for OAS 3.0).

[brandonbloom]

I didn't try OpenAPIKit30, but I can. Investigating your comment about removal of this feature lead me to this: OAI/OpenAPI-Specification#2244

It appears that the community is pretty confused about this. I don't know of OpenAPI and/or JSON Schema follow SemVer or not, but removing support for such a commonly used property in a dot release seems like an unwelcome change. There are many examples of openapi config files with the 3.1.0 version number still using nullable. See: https://github.com/search?type=code&q=path%3Aopenapi.json+nullable+3.1.0 and https://github.com/search?type=code&q=path%3Aopenapi.yaml+nullable+3.1.0

To me, this is an area where it is best to follow the "robustness principle". Maybe support with a warning is appropriate?

[mattpolzin]

Heads up, I had a lot to say here. Stick with my rambling if you don't mind, I have no ill will, just thoughts on maintaining tools like OpenAPIKit.

I don't know of OpenAPI and/or JSON Schema follow SemVer or not [...]

They don't. They documented this in their earliest RC release notes saying "As part of this release, we have decided to not follow SemVer anymore, and as such introduce breaking changes." I don't have a strong opinion on them deciding not to follow sem-ver, but then in other places they seem to be more wishy-washy about it:

The OpenAPI Initiative had adopted semantic versioning [...]. Technically, using semantic versioning with the new full alignment with JSON Schema would require this change to be denoted as 4.0.0. However, this update to OpenAPI important improvements, specifically the alignment with JSON Schema, but to force it into a major release numbering would have created a mismatch of expectations.

The wishy-washiness and general community confusion seem to both lead to the conclusion that they didn't manage the release very well from a communication perspective if nothing else.

To me, this is an area where it is best to follow the "robustness principle". Maybe support with a warning is appropriate?

That's a tough sell for me. I want OpenAPIKit to be forgiving when possible, but that goal can be at odds with another goal of mine: As much as possible, OpenAPIKit should produce the same document it ingested modulo formatting (if you decode a document with OpenAPIKit and then encode the result again). If we allow decoding nullable and then also encode it as nullable on the way out, we actively participate in the confusion over nullable being invalid and possibly produce an output that other strict v3.1.0 tooling will not accept. On the other hand, if we decode nullable but then encode it as type: ["null"] on encoding then we produce a document different than we ingested and possibly break compatibility with whatever tooling the user was using that created their nullable properties in the first place.

OpenAPIKit has always been strict on purpose because I want to help push the OpenAPI community in the direction of consistency, encouraging bug reports against non-conformant tools and encouraging fixes to non-conformant documents. I don't really want to be in the business of deciding which OpenAPI specification decisions (in this case nullable keyword support) OpenAPIKit chooses to be flexible about because that creates a maintenance burden on this project and potentially sets end-users of tools written with this project up for problems downstream where some other tool refuses to accept their schema or even worse it ignores the nullable property quietly so the tooling produces unexpected results possibly without the end-user noticing.

---

All of that said, if a problem is big enough, I'm not going to ignore it. I might consider adding support for nullable decoding behind a feature flag so that it could be optionally turned on and tools using OpenAPIKit could choose to allow it (albeit still with a warning). Then tools using OpenAPIKit could choose whether or not to be lenient about this particular property. That still introduces a maintenance burden, but I'm not totally opposed to it.

[mattpolzin]

Ok, I dug into your linked GitHub searches and the issue opened against the OAS project. Granted your GitHub searches do find examples of non-conformant OAS 3.1 documents, but the linked GitHub issue seemed to be full of references to other projects where mostly maintainers were explaining why the project would switch from supporting nullable to supporting type: ["null"] for OAS 3.1 rather than projects where the decision was made to support the removed nullable property in addition to the type array. A couple of the links were to issues in other projects that were never addressed, one or two mentioned that the problem and solution would live in upstream dependencies, and the rest mentioned that the new spec would be adopted without backwards support for nullable as best I could tell.

[brandonbloom]

I'm sensitive to all of your thoughts/points. Some specific comments below.

As much as possible, OpenAPIKit should produce the same document it ingested modulo formatting (if you decode a document with OpenAPIKit and then encode the result again).
[...]
if we decode nullable but then encode it as type: ["null"] on encoding then we produce a document different than we ingested and possibly break compatibility with whatever tooling the user was using that created their nullable properties in the first place.

The robustness principle guidance would be to normalize on output. Yes, this violates the desire to preserve round-tripping, but it also helps your other goal:

OpenAPIKit has always been strict on purpose because I want to help push the OpenAPI community in the direction of consistency, encouraging bug reports against non-conformant tools and encouraging fixes to non-conformant documents.

If they don't support the type union syntax, they'll definitely get bug reports :)

If OpenAPIKit don't support the schemas that are in the wild, folks are less likely to use it, and therefore less likely to be exposed to your preference for consistency.

some other tool refuses to accept their schema or even worse it ignores the nullable property quietly so the tooling produces unexpected results possibly without the end-user noticing.

I think that this one is pretty likely for the end-user to notice. Any decoder that expects a string for the type keyword would immediately hit an error or otherwise totally muck up and validation or code generator implementation.

If we allow decoding nullable and then also encode it as nullable on the way out, we actively participate in the confusion over nullable being invalid and possibly produce an output that other strict v3.1.0 tooling will not accept.

I don't think this is the right thing to do simply in the name of round-tripping support, other than maybe by config/option if desired. My suggestion would be to relax the goal to: "OpenAPIKit should produce the same document it ingested modulo formatting for conforming schemas" and maybe to also add "and to produce warnings for any non-conforming schemes".

[mattpolzin]

I think I am beginning to lean more toward your proposal of allowing nullable but warning. That would mean that OpenAPIKit parsed nullable, but the default validate() (which is a strict validation by default) would fail because it disallows warnings to have been produced. You don't need to run validation, much less strict validation, so that's perfectly out of the way for the end user use-case of "just give me something meaningful for this document if you can."

If OpenAPIKit don't support the schemas that are in the wild, folks are less likely to use it [...]

I think this is potentially true, but not one of my bigger concerns. First of all, the ecosystem is small especially within a particular language like Swift. Second of all, I see other OAS implementations being strict as well so the choices out there that are less strict and still somewhat good drop-in replacements is even smaller yet. Lastly, I am a heavy user of this library myself, and when I would hit non-starters with OAS documents I was trying to use (because OpenAPIKit refused to decode them) I would just go off and create a fork of the document repository and a file a bug request with the author of that document. I'd switch to the fork and almost always see my bug request or PR resolved fairly quickly (at least by companies that truly wanted to support their OAS documents). This even happened a few times with big players like GitHub and they were always happy to fix their specifications. That's what I mean when I say I'd like to "help push the OpenAPI community:" I want to encourage people to help each other out with bug reports instead of shrugging their shoulders at a warning.

Still, like I said at the top of this message, I think you are swaying me. I realize I am an idealist sometimes and in those cases I usually just need to see a conversation through like this one to start to change my mind. Give me a day.

Would you be interested in adding that warning in this PR assuming I keep coming around to the idea? It'd be a bit of piping since the closest type with the HasWarnings conformance is JSONSchema currently.

[mattpolzin]

I opened a PR against this branch that pipes the aforementioned warning through. If that sounds like what you were suggesting, go ahead and merge it and then we can merge this PR.

ref. brandonbloom#1

[brandonbloom]

That looks sensible to me. Thank you! Merged to this PR for further merging.