use `nullable` OpenAPI property to determine nullability #360

nlundquist · 2021-02-18T18:56:37Z

Basic attempt at using nullable instead of required to determine nullability. Fixes #359

My use case only deals with querying data so I removed required, however it may be still relevant when creating input/mutation types.

Alan-Cha · 2021-02-19T17:20:27Z

Thanks for filing the issue and the pull request! This looks good to me! Can you also sign off your commits?

git rebase HEAD~5 --signoff

In addition to this, we will also need to add some test cases and fix all the old ones. I can take a crack at this if you do not have the time. Just let me know!

nlundquist · 2021-02-22T03:13:49Z

Oh sure I'll sign off on them from my work machine tomorrow. I probably won't have the time to update the test cases in the next few days, this patch has unblocked some pressing work I'm perusing. I'd appreciate the help with the test suite.

nlundquist · 2021-02-24T17:27:04Z

There I believe those are now signed off. Apologies for the delay.

Alan-Cha · 2021-02-25T16:41:40Z

Thanks for getting back to me! I think there are still a few commits that need to be signed off, name e600a79, 7c84104, and 1d84f1e.

And I'll get started looking into the test cases!

…ility Signed-off-by: Nils Lundquist <[email protected]>

Signed-off-by: Nils Lundquist <[email protected]>

Alan-Cha · 2021-03-02T07:10:31Z

After doing some more research, I wonder if the proposed change is too simple.

I'm still new to the conversation but it seems that historically, there seems to be a lot of debate over how nullability should be conveyed, either using nullable or specifying that the type can be null. This debate is quite intricate and can be confusing. From the surveys we've performed, it's clear that not all OAS are created the same (i.e. are of the same quality), so I doubt that everyone is using these properties consistently and properly. With the current changes, we are not taking these alternatives into consideration.

Another one of my concerns stems from the fact that the OAS by default assumes that nullable is set to false. This PR follows that precedent but with the current changes, this will break a lot of schemas due to cyclical relationships. It would require a lot of work to check for cyclical relationships and modify the schema to reflect that.

Arguably, it will be much easier and more functional if we just assume all fields to be nullable.

I was thinking we can check if nullable is explicitly set to false to create non-null fields, but I wonder if this is idiosyncratic, given that OAS assumes it to be set to false.

In any case, I am proposing that we remove the required as originally intended but we do not replace it with a nullable check. Perhaps we can check for nullable is explicitly set to false to create non-null fields but that could be a future enhancement. Thoughts?

nlundquist · 2021-03-02T22:06:26Z

nullable: true is the OpenAPI 3.0 and prior way of expressing nullability.

type: [string, whatever, null] is the OpenAPI 3.1 and forward way.

This is a result of the replacing the prior "it's pretty much JSONSchema" schema language with full-fat JSONSchema.

I'm adverse to the idea of not handling this according to the spec. IMO nullability is important to model as accurately as possible.

In my use case for example the generated schema is used to provide documentation to users via GraphQL Playground & GraphQL Voyager. I wouldn't want consumers of my API handling nulls everywhere thinking that every field is actually nullable.

Granted, my use case is pretty different than the typical user of this package, I'm not using the openapi-to-graphql server just the schema generator (though I do more with it than just documentation).

Aside from that, making every field nullable allows users to hit APIs with invalid null fields rather than catching them during the GQL mutation validation. Not the end of the world, but notable.

I wouldn't want to make this opt-in by explicitly setting nullable: false since it wouldn't be something the average OpenAPI would typically do. Making this change across all the teams my OpenAPI is composed from would be a colossal task.

Can you link to a public example of an organization who's not using nullable as they should, leading to the unintentionally infinitely recursively schemas you mention? Perhaps the specs afflicted with this issue are older or out of date specifications and putting this behavior behind an OpenAPI version check is all that would be needed.

If there's no other reasonable way, than I'd support making this behavior opt-in via a boolean option passed to openapi-to-graphql.

Alan-Cha · 2021-03-03T07:10:56Z

@nlundquist You make a lot of convincing points.

Can you link to a public example of an organization who's not using nullable as they should, leading to the unintentionally infinitely recursively schemas you mention? Perhaps the specs afflicted with this issue are older or out of date specifications and putting this behavior behind an OpenAPI version check is all that would be needed.

I was originally concerned because a lot of the tests are now currently broken but after some careful inspection, I believe that these are errors with the example OASs that I created, which I blame on my own inexperience.

To verify that OtG can still successfully wrap the vast majority of OASs in GraphQL, I reran the APIs.guru evaluation. Here are my findings...

Current behavior:

----------------------
Overall results:
Assessed APIs: 1885
Successes: 1626
  with no warnings: 115
Errors: 259
----------------------
Warnings breakdown:
{
  "MISSING_RESPONSE_SCHEMA": 6352,
  "OBJECT_MISSING_PROPERTIES": 1166,
  "OAUTH_SECURITY_SCHEME": 1299,
  "UNKNOWN_TARGET_TYPE": 372,
  "MULTIPLE_RESPONSES": 9181,
  "UNRESOLVABLE_SCHEMA": 126,
  "DUPLICATE_FIELD_NAME": 11,
  "DUPLICATE_OPERATIONID": 1
}

Proposed non-nullable behavior:

----------------------
Overall results:
Assessed APIs: 1885
Successes: 1626
  with no warnings: 108
Errors: 259
----------------------
Warnings breakdown:
{
  "MISSING_RESPONSE_SCHEMA": 6352,
  "OBJECT_MISSING_PROPERTIES": 1162,
  "OAUTH_SECURITY_SCHEME": 1299,
  "UNRESOLVABLE_SCHEMA": 1484,
  "UNKNOWN_TARGET_TYPE": 359,
  "MULTIPLE_RESPONSES": 9181,
  "DUPLICATE_FIELD_NAME": 11,
  "DUPLICATE_OPERATIONID": 1
}

So it seems that we are still able to successfully wrap the same number of OASs from the wild. There are differences in the warnings but I wouldn't read into them too much because this PR needs to be rebased (for example, you fixed the .properties bug which is related to the UNRESOLVABLE_SCHEMA warning).

This makes me more confident that this is a change that we can accept but I am still wary about some its implications, for example with input object types. I've asked my colleague @ErikWittern to give his opinion on it. I'd like to hear his opinions before moving further with this PR.

If we feel that my concerns do hold some water, then I think we should add this as an opt-in option as your suggested or wrap it along with the strict option.

Alan-Cha · 2021-03-05T20:19:02Z

Okay, I've heard back from @ErikWittern and we're going to go through with this change. I still need to make a lot of changes to the tests so this will take some time.

ibm-ci-bot · 2021-07-15T17:48:48Z

@nlundquist: PR needs rebase.