Fix required attribute by considering schemas. #586

rustyconover · 2021-05-06T00:21:18Z

Resolves #584

drwpow · 2021-05-07T15:07:54Z

src/utils.ts

+function resolveDocumentReference<T>(document: any, reference: string): T | undefined {
+  if (reference[0] === "#") {
+    const parts = reference.replace(/^#\//, "").split("/");
+    const result = _.get(document, parts.join("."));


I’d like to not add lodash to this library all for one function. Why not use optional chaining instead?

I've changed to the single get function export of lodash.

I'd rather not write some code to dynamically walk a JS object when lodash already has a function that already has it and is well tested.

I know lodash isn’t gigantic, but aren’t you just using optional chaining here? It’s more about adding weight for something built into the language now.

Considering the paths are dynamic based on the contents of the ref, could you show me an example, I don't think optional chaining does what I think lodash's get method does.

It seems to me that together we are going going to iterate the parts of the path, with a current value then traverse down using each property name. Optional chaining does not do that unless you write the property path explicitly in the code which we can't do because references are dynamic in both length and content.

haven't looked at the details of the above, but you can use expressions with optional chaining:

const foo = 'x-foo' obj?.[foo]?.['bar'].x

not sure how I'd approach the length, possibly with a loop ...

codecov · 2021-05-07T15:07:57Z

Codecov Report

Merging #586 (ebcc07d) into main (a36a728) will decrease coverage by 0.55%.
The diff coverage is 74.41%.

❗ Current head ebcc07d differs from pull request most recent head dd38fbc. Consider uploading reports for the commit dd38fbc to get more accurate results

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
- Coverage   88.53%   87.97%   -0.56%     
==========================================
  Files           9        9              
  Lines         349      391      +42     
  Branches      123      143      +20     
==========================================
+ Hits          309      344      +35     
- Misses         37       47      +10     
+ Partials        3        0       -3

Impacted Files	Coverage Δ
src/transform/index.ts	`83.92% <ø> (+4.29%)`	⬆️
src/transform/responses.ts	`88.37% <ø> (+0.27%)`	⬆️
src/transform/headers.ts	`23.07% <25.00%> (-4.20%)`	⬇️
src/transform/schema.ts	`95.00% <70.00%> (-2.06%)`	⬇️
src/transform/parameters.ts	`91.66% <75.00%> (-5.84%)`	⬇️
src/utils.ts	`91.17% <84.61%> (-1.56%)`	⬇️
src/transform/operation.ts	`97.05% <100.00%> (+0.18%)`	⬆️
src/transform/paths.ts	`92.30% <100.00%> (+0.30%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 787eb1e...dd38fbc. Read the comment docs.

drwpow · 2021-05-07T15:12:11Z

src/transform/parameters.ts

@@ -34,6 +36,8 @@ export function transformParametersArray(
        } else if (version === 3) {
          mappedParams[reference.in][reference.name || paramName] = {
            ...reference,
+            // They want to keep this ref here so it references
+            // the components, but it does violate the standards.


Could you explain a little more about how this violates the standards? Should this be removed?

Consider a document that has a reference as for a parameter to #/components/parameters/foobar.

This code requires the schema attribute to be that reference, but it turns out the code that I've written for determining if there is a default value for the parameter expects the schema for the parameter itself to be in the schema property that is getting written here.

So to access the true schema of the parameters the code has to follow .schema.schema, which seems quite awkward to reason about but this PR does implement it.

Ah yes for parameters we did try and flatten the schema, because in the TypeScript conversion we’re more interested in types, and we found the interface easier to use without nested .schema … .schema everywhere. I don’t know if I would say this “violates the standards” because TypeScript types are not bound to the standards. There are some opinions and interpretations we’re making here (of course, we try and minimize those, but TypeScript is ultimately a different language so there are always some translations that must happen).

I guess in a related question: what’s the purpose for this comment? Is this a TODO? Does this clarify the purpose of the existing code? I’m a little unclear why this was added when no other code here was changed.

It was a note to myself, you can merge it or not merge it as you like. But it may help other users who want to edit this code understand what is going on.

drwpow · 2021-05-07T15:13:04Z

src/transform/schema.ts

+      }
+    }
+
+    output += `${readonly}"${key}"${required.includes(key) || hasDefault ? "" : "?"}: `;


👍🏻 Thanks for the fix!

drwpow · 2021-05-07T15:15:00Z

src/transform/schema.ts

+
+    let hasDefault = false;
+    if (value.schema != null) {
+      const schema = resolveRefIfNeeded<SchemaObject>(options.document, value.schema);


Is passing the entire document necessary here? Could we simply add more items to thatrequired[] array (if they have default values)? That way we’re figuring this stuff out in the context that has that information, and not at the schema object level. But happy for pushback if that‘s simply not possible.

The default value is set on the actual values themselves, as such it seems that the PR follows most closely to your goal. I think manipulating the required array would certainly be not as clear.

Additionally, if the required array isn't specified there would be special code to handle that.

If it’s a problem of the required array being nullable, we could fix that. From what I hear you saying, it would be possible to modify the required array to solve this problem. If that’s the case, I vote for doing that instead. I disagree that this is “clearer;“ this PR does take a module which previously was only worried about a single schema object at a time, and now this module has complete document context and so now its responsibilities are less clear.

Also, as an aside, this library does support partial schemas, so I’d like to avoid handling that logic in two places (here and at the top level) if possible.

If it’s simply an issue of required being too unclear a name, I vote for renaming that array. Or have it be an array with more data in there about default properties so that we don’t need to tunnel the document through multiple levels of modules.

@drwpow You could do what you want by manipulating the required array. It's up to you what happens from here. I think my PR is clearly demonstrating the changes that are necessary to be more correct about what fields are required and which ones aren't. If you want to resolve the problem another way great, I'd be happy to test it.

drwpow · 2021-05-07T15:15:52Z

Thanks for the fix! I’m on board with solving the overall problem and I like the general direction. Just have a few questions / feedback before merging.

Resolves openapi-ts#584

rustyconover · 2021-05-09T20:10:59Z

I've just rebased against the main branch with these latest commits so it should apply cleanly.

drwpow · 2021-05-25T22:36:44Z

@rustyconover sorry for the noise created from the recent changes. After digging further into the issue you raised, I align more and more with your approach. But I’ve decided I want to release remote schemas first (#602). So I’ve opened up #613 as a half-measure for what you’ve done here. I’d like for you to give it a look and see if you agree with it, pointing out that you handled $ref resolution whereas I didn’t.

How do you feel about releasing an incremental update for now, then coming back to resolving $refs later in a world of remote schemas?

Like your PR, the final solution would introduce a lot of complexity, but would generate accurate types. But that said, I’d be worried that merging this PR now would mean that remote schemas wouldn’t happen, as this would likely have to get rewritten.

This commit makes headers by included in the produced types if they are included by reference.

rustyconover · 2021-05-30T14:32:12Z

Hi @drwpow I've just added fixes for headers being included by reference that weren't being properly handled.

You can see more of the details in: #624

This changes the use of the any type to the unknown type with requires asserting types or type narrowing. This promotes more explicit handing of results and less surprises.

…ut does have Any/One/All Of.

drwpow · 2021-06-26T03:13:16Z

Sorry about the conflicts in the main branch; this PR just happened to get opened as remote $refs were getting merged.

As a general direction, I still don’t want to keep all the schemas in memory as you’re doing and passing them through to every function (especially as they’re getting transformed).

Instead, I’d like to see a solution that does that initial work up-front, and saves it on the property itself. For example, what if you did the work in load.ts, and saved everything to an x-oapi-ts-required property like so?

// Note: this would have to happen before the $refs are transformed into TypeScript refs, i.e. around LN#127
// (for bonus points, this could happen synchronously while remote schemas are being downloaded async)

JSON.parse(JSON.stringify(schemas[subschemaURL]), (k, v)) => {
  if (!v.$ref) return v; // select parent node of $ref;
  const { url: refURL, parts } = parseRef(v);
  if (refURL) return v; // don’t try and resolve remote schemas (for now)

  // find referenced node, if we can
  let foundRef: any = schemas[subschemaURL];
  for (const key of parts) {
    if (!foundRef[key]) return v;
    foundRef = foundRef[key];
  }

  // check if the referenced node is required
  if (foundRef.required) v['x-oapi-ts-required'] = true;
  return v;
});

Then in transform/schema.ts you should only need to check for that x-oapi-ts-required property. This should be less far-reaching, and easier to work with with remote schemas.

Does this seem like a good suggestion?

fkowal · 2021-12-21T13:25:57Z

@drwpow @rustyconover

this fix resolves the issue i've got in my project

I wanted to ask if there is anything I can help with to resolve this issue and get this released

PR seem stagnant

rustyconover force-pushed the feature-check-defaults-for-required branch from 39f80d8 to d1bf493 Compare May 6, 2021 03:59

drwpow reviewed May 7, 2021

View reviewed changes

rustyconover added 2 commits May 9, 2021 16:09

Fix required attribute by considering schemas.

3292d31

Resolves openapi-ts#584

Remove lodash dependency, change to lodash.get for smaller import size.

ebcc07d

rustyconover force-pushed the feature-check-defaults-for-required branch from b75354f to ebcc07d Compare May 9, 2021 20:10

Fix handling of headers that are specified via reference.

9c21864

This commit makes headers by included in the produced types if they are included by reference.

rustyconover added 2 commits May 30, 2021 10:54

Fix: change uses of "any" to "unknown".

54c10d0

This changes the use of the any type to the unknown type with requires asserting types or type narrowing. This promotes more explicit handing of results and less surprises.

Don't emit additional properties when the node does not have a type b…

dd38fbc

…ut does have Any/One/All Of.

drwpow mentioned this pull request Jun 26, 2021

Advanced required enforcement for allOf + required case #657

Closed

njanor mentioned this pull request May 18, 2022

Bug: Headers that use refs aren't included in output #624

Closed

drwpow closed this Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix required attribute by considering schemas. #586

Fix required attribute by considering schemas. #586

rustyconover commented May 6, 2021

drwpow May 7, 2021

rustyconover May 9, 2021 •

edited

Loading

drwpow May 10, 2021

rustyconover May 11, 2021

dnalborczyk May 26, 2021

codecov bot commented May 7, 2021 •

edited

Loading

drwpow May 7, 2021

rustyconover May 9, 2021

drwpow May 10, 2021 •

edited

Loading

drwpow May 10, 2021

rustyconover May 11, 2021

drwpow May 7, 2021

drwpow May 7, 2021 •

edited

Loading

rustyconover May 9, 2021

drwpow May 10, 2021 •

edited

Loading

rustyconover May 11, 2021

drwpow commented May 7, 2021

rustyconover commented May 9, 2021

drwpow commented May 25, 2021 •

edited

Loading

rustyconover commented May 30, 2021

drwpow commented Jun 26, 2021 •

edited

Loading

fkowal commented Dec 21, 2021

Fix required attribute by considering schemas. #586

Fix required attribute by considering schemas. #586

Conversation

rustyconover commented May 6, 2021

Choose a reason for hiding this comment

rustyconover May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 7, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow commented May 7, 2021

rustyconover commented May 9, 2021

drwpow commented May 25, 2021 • edited Loading

rustyconover commented May 30, 2021

drwpow commented Jun 26, 2021 • edited Loading

fkowal commented Dec 21, 2021

rustyconover May 9, 2021 •

edited

Loading

codecov bot commented May 7, 2021 •

edited

Loading

drwpow May 10, 2021 •

edited

Loading

drwpow May 7, 2021 •

edited

Loading

drwpow May 10, 2021 •

edited

Loading

drwpow commented May 25, 2021 •

edited

Loading

drwpow commented Jun 26, 2021 •

edited

Loading