Mark schema objects with default values as non-nullable #613
Conversation
Partially fixes #584
Codecov Report
@@ Coverage Diff @@
## main #613 +/- ##
==========================================
+ Coverage 88.53% 90.25% +1.71%
==========================================
Files 9 10 +1
Lines 349 390 +41
Branches 123 141 +18
==========================================
+ Hits 309 352 +43
- Misses 37 38 +1
+ Partials 3 0 -3
Continue to review full report at Codecov.
|
|
Looks awesome! |
| @@ -15,10 +15,14 @@ interface TransformSchemaObjOptions extends GlobalContext { | |||
| required: Set<string>; | |||
| } | |||
|
|
|||
| function hasDefaultValue(node: any): boolean { | |||
| if (node.hasOwnProperty("default")) return true; | |||
There was a problem hiding this comment.
This is the only change here: check if a schema object has a default value. But like the description states, we can’t currently look this up for $ref (which might be a remote schema)
| @@ -72,7 +72,7 @@ export interface components { | |||
| shipDate?: string | |||
| /** Order Status */ | |||
| status?: 'placed' | 'approved' | 'delivered' | |||
| complete?: boolean | |||
| complete: boolean | |||
There was a problem hiding this comment.
Updated snapshots show it’s improved a lot of schemas
|
Do you mind if we revert this change until we figure out to differentiate between request parameter types and response types? This change made a bunch of I don't think this is a simple fix |
|
Hi Gregor,
I disagree a bit with you, I believe this change is correct for my use
cases but I’d love to understand your perspective more.
I’m considering the types from the perspective of a server implementing the
API (a handler function that runs after parsing and validating) and from
that perspective the values are required because they will be defined when
passed into the handler. The default values will be set.
If we’re using the types from a client you’re right the values don’t have
to be required, since the server should set the default values.
Maybe there needs to be a switch for the perspective of the interface?
Indicating client or server based types would be the description of the
option.
Rusty
…On Fri, May 28, 2021 at 1:48 AM Gregor Martynus ***@***.***> wrote:
Do you mind if we revert this change until we figure out to differentiate
between request parameter types and response types? This change made a
bunch of requestBody parameters from GitHub's OpenAPI spec required
because there is a defaults key, but that is incorrect. The parameters
are not required, the spec just states what value the parameter will get if
it is not set.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#613 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFSWJITV5UFDX3VO4DQF6TTP4VD3ANCNFSM45QK6STQ>
.
|
|
@gr2m thanks for your feedback! And apologies for the changes to the GitHub schema. I too am a bit on the fence about this one, because this is an area where there’s overlap between type generation and business logic, and the lines are a bit blurry. Do these types sit between the API response and your business logic? If so, they should probably be stricter, and err on the side of more nullable properties as they were before so that you can fill in the defaults. Or do these types sit between the business logic and your frontend, after the defaults have been applied? If so, the recent change to respect I’m sure people use types in different ways according to their different needs, which leads to differences in interpreting OpenAPI -> TypeScript. If this is causing issues for people, I’d rather err on the side of doing what the library was doing before, revert the behavior, and make this behavior opt-in via a flag (e.g. |
|
GItHub's OpenAPI Spec is for public consumption. They use it internally as well, but it's also used to generate the docs at https://docs.github.com/en/rest/reference. So if a default value is set on a request parameter, it cannot be interpreted as a required parameter. I use the OpenAPI spec to generate several SDKs (e.g. https://github.com/octokit/plugin-rest-endpoint-methods.js/), and they types (github.com/octokit/openapi-types.ts and github.com/octokit/types.ts), so I have the API consumer perspective.
yes that sounds like a good plan |
|
@gr2m Fixed in |
Partially fixes #584.
This is a very good fix pointed out by @rustyconover, and handled in PR #584, but that currently would be blocking remote schemas, as
$refs would no-longer be resolvable (or at least, not easily). This PR does half of #584, where$refs are not resolved. But it does improve the handling ofdefault:without blocking remote schemas.