[Schema Inaccuracy] code-scanning-alert-dismissed-at has null in enum

[jessfraz]

Schema Inaccuracy

Expected

Since you already set the type as nullable do not include "null" in the enums, this is breaking some parsers.

[xuorig]

Thank you for opening this and the other issues @jessfraz. This one is tricky because of the semantics of nullable.

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value.

More context in this issue: OAI/OpenAPI-Specification#1900

https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#fixed-fields-20

Can you point me to the parsers that are breaking? null is valid within a JSON schema enum. Maybe the parsers in question require a fix. See for example the second snippet here: https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values

I don't believe this is a problem with our description, so I'm going to close this one. Feel free to reopen with new discoveries!

[jessfraz]

ugh yeah thats definitely breaking the rust openapi library because it is very strongly typed so having a null with a bunch of strings is a panic

[xuorig]

Ugh yeah I actually personally hit that issue last week working on https://github.com/xuorig/anicca 🤦 I implemented it as Vec<Option<EnumValue>> for now but it isn't really ideal. Unfortunately it's really in the spec 😭

[jessfraz]

oh sweet maybe ill just use your lib? is that like a modified fork openapiv3? or your own?

…

On Thu, Jul 15, 2021 at 11:48 AM Marc-Andre Giroux ***@***.***> wrote: Ugh yeah I actually personally hit that issue last week working on https://github.com/xuorig/anicca 🤦 I implemented it as Vec<Option<EnumValue>> for now but it isn't really ideal. Unfortunately it's really in the spec 😭 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALA23DGYMV3MHQAX3SYR3LTX4UQRANCNFSM5AMIVCNQ> .

-- Jessie Frazelle 4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3 pgp.mit.edu <http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3>

[xuorig]

oh sweet maybe ill just use your lib? is that like a modified fork
openapiv3? or your own?

Most of it is from openapiv3 but there's a couple fixes like this + a completely different approach to the JSON schema representation. It uses a flat schema with all options rather than the enum approach openapiv3 uses. The idea is awesome but JSON schema is too complex to be represented that way / the enum model from that crate doesn't match the actual spec.

I'd be happy to clean up that module / extract it / make it more reusable. Let me know!

[jessfraz]

I've been building this ;) https://docs.rs/octorust/0.1.13/octorust/index.html Theres a few fields missing but over all, already consuming it and it works which is good And yeah I agree with you completely my code looks like shit because of openapis use of enums, but then it kinda made me do some clever things with enums in my lib but yes I completely agree :) will check out your lib On Thu, Jul 15, 2021 at 11:50 AM Jess Frazelle ***@***.***> wrote:

…

oh sweet maybe ill just use your lib? is that like a modified fork openapiv3? or your own? On Thu, Jul 15, 2021 at 11:48 AM Marc-Andre Giroux ***@***.***> wrote: > Ugh yeah I actually personally hit that issue last week working on > https://github.com/xuorig/anicca 🤦 I implemented it as > Vec<Option<EnumValue>> for now but it isn't really ideal. Unfortunately > it's really in the spec 😭 > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < #454 (comment) >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AALA23DGYMV3MHQAX3SYR3LTX4UQRANCNFSM5AMIVCNQ > > . > -- Jessie Frazelle 4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3 pgp.mit.edu < http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALA23D4UIZBES6UY7AP2WLTX4UX7ANCNFSM5AMIVCNQ> .

-- Jessie Frazelle 4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3 pgp.mit.edu <http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3>

[xuorig]

https://docs.rs/octorust/0.1.13/octorust/index.html

Awesome 😍

[jessfraz]

Actually I'd appreciate your take on something...

So for repos().get_content in my generated API: https://docs.rs/octorust/0.1.13/octorust/repos/struct.Repos.html#method.get_content Where it returns a oneOf from the spec, I used an enum, but after implementing something that consumes this library I hated it, and it was flaky because sometimes it mistakes one type for another because I don't have very strict serde rules I actually have extremely not strict serde rules so that things parse without errors. ANyways bit me in the ass there but its over all worth it

SO I also injected more functions: https://docs.rs/octorust/0.1.13/octorust/repos/struct.Repos.html#method.get_content_vec_entries https://docs.rs/octorust/0.1.13/octorust/repos/struct.Repos.html#method.get_content_file etc for each of the response types

Anyway, I was wondering what you all do in other clients you generate, javascript is likely a lot easier but go, I assume would be the same issue

[gr2m]

@jessfraz amazing that you are building an Octokit client in Rust! If you have any questions please let me know! I kinda sorta started writing a handbook for people building SDKs for GitHub's platform APIs with all my learnings building the JS one: https://github.com/octokit/handbook/

I was wondering what you all do in other clients you generate

In the JavaScript Octokit Types, we return a union, so you have to narrow down the response data type with checks before using it. Folks are tripped up by it constantly, not understanding why the TypeScript compiler is complaining when they just want to user response.data.content. But it's correct.

The way I'd approach it is to keep the client as close as possible to the actual APIs with all it's complexities, but then create plugins / wrappers for specific use cases that hide away these complexities.

E.g. I created https://github.com/octokit/plugin-create-or-update-text-file.js/ which adds a octokit.createOrUpdateTextFile method with a simple API, and you don't need to worry about the different types of responses the repository contents API may give you.

I'm not familiar with the strict typing in Go or Rust I'm afraid so I'm not sure what the right approach for higher level abstractions would be in these cases

[xuorig]

Thanks for chiming in @gr2m.

As far as implementing anyOf... it's hard. Technically to avoid having to use Serde untagged we should work on including discriminators in this description. But these require actual runtime changes to the API which hasn't been prioritized / requires a bit more thought than changes to the OpenAPI description.

Technically with discriminators we'd be able to use internally tagged serde enums (Although names would need to match perfectly if used naively 🤔)

Certainly something to explore. Overall we're careful about adding too many complex schemas going forward. They aren't easy to implement, validate, understand, or generate docs from + they really sometimes are a smell for sub-par API design. Still valid use cases for them though :)

[jessfraz]

Ah okay thats helpful thanks @gr2m and @xuorig :)