Fix misplaced description property if refSiblings = allOf #287

ideadapt · 2020-09-30T07:20:29Z

I think there is a bug with refSiblings = allOf and description next to $ref property in a swagger input schema.

# swagger input
  a:
    $ref: '#/definitions/b'
    description: a
# wrong output
   a:
      allOf:
      - $ref: '#/components/schemas/b'
        description: a
# fixed output
   a:
      allOf:
      - $ref: '#/components/schemas/b'
      description: a

A description property next to a $ref property should be moved next to new allOf (and not into allOf array). Only e.g. an existing properties object (or any other sub schema) next to a $ref property should be moved into allOf array (see also https://json-schema.org/understanding-json-schema/structuring.html#id5).

See also #130

MikeRalphson · 2020-10-01T13:58:48Z

Thanks for the PR. Let me add a test for the existing behaviour of all the refSiblings option values and then you can rebase and see where we are.

MikeRalphson · 2020-10-01T13:59:49Z

packages/swagger2openapi/index.js

@@ -307,7 +307,11 @@ function fixupRefs(obj, key, state) {
            }
            else if (inSchema && (options.refSiblings === 'allOf')) {
                delete obj.$ref;
-                state.parent[state.pkey] = { allOf: [ { $ref: tmpRef }, obj ]};
+                if(typeof obj.description === 'string'){


Spaces between if and ( please. refSiblings should deal with all sibling properties of a $ref, not just description (or special-case it).

I was not sure how other properties should be treated, see commit message 3892da8. I am sure that description is a special case, thats why I implemented it as such. All other cases (like a nested properties object) still work the same way as before, and the way JSON schema defines.

Why is description a special case, in OAS or JSON Schema?

I think in terms of JSON Schema the final state using the allOf trick is valid. In terms of OAS, if we do not do this transformation all siblings of $ref are ignored, see https://swagger.io/docs/specification/using-ref/ at the end.
So maybe you are right, we do not have to consider all possible valid JSON Schema scenarios as input here. It's enough to only consider valid swagger inputs. Hence, there will never be a sibling of $ref called "properties" of type object. But we should move all siblings of $ref next to allOf (not only description sibling).
Was this more or less understandable :/ ? What do you think?

There's two schools of thought on this, a one-element allOf with a $ref inside it to side-step the no-ref-siblings rule, or to use a multi-element allOf to indicate that both this (the $ref) and that (any other ex-sibling keywords) need to be evaluated.

You can probably tell from the existing code and the test outputs what my preference is. I'm not really interested in creating a special case just for description, or required or additionalProperties etc etc.

But in your changed test output you are not "lifting" the sibling properties like description anywhere, you are simply burying a $ref in a one-element allOf and leaving description where it was.

If "swagger tools" ignore additional elements of an allOf that is their problem, and IMHO a bug.

One way out of this would be if you could find a good name for it, to specify a new value for refSiblings which uses the one-element allOf form (i.e. only the $ref gets moved into an allOf if we are inside a schemaObject).

As far as I can tell, the existing refSiblings === 'allOf' logic is already only executed in schema context. We are using https://github.com/byu-oit/openapi-enforcer to validate the generated output. This validation fails, if I use refSiblings === 'allOf' without my fix. If I do not set refSiblings (leaving at the default value), then the generated output is valid, but the generated HTML preview (using redoc) does not contain any description information, see screenshot:

The redoc behaviour seems ok to me, since the swagger docs state that any $ref siblings should be ignored.

This is why I thought it's legitimate to fix it that way. Because when I apply my fix, openapi enforcer validation is ok, and the rendered HTML doc using redoc renders the description.

Now you're talking about other (non-swagger) tools. If ReDoc doesn't respect the second and subsequent branches of an allOf IMHO that's a bug in ReDoc.

If openapi-enforcer says an allOf with more than one element is not valid, then that is definitely a bug in openapi-enforcer.

npx openapi-enforcer-cli validate issue130-allOf/openapi.yaml

One or more errors exist in the OpenApi definition at: components > schemas > a > allOf > 1 Missing required property: type

That's definitely a bug in openapi-enforcer, type is not a required property in a schemaObject. And it doesn't seem to have much to do with this PR.

I think openapi enforcer does its job. According to https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#allof allOf holds an array of schemas. This is also what https://github.com/OAI/OpenAPI-Specification/blob/3.0.1/versions/3.0.1.md says, as far as I understand.

openapi-enforcer says that only (sub)schemas are valid as allOf elements (according to openapi spec). redoc correctly ignores any $ref siblings (according to swagger spec).

MikeRalphson · 2020-10-01T14:00:09Z

packages/swagger2openapi/index.js

-                state.parent[state.pkey] = { allOf: [ { $ref: tmpRef }, obj ]};
+                if(typeof obj.description === 'string'){
+                    state.parent[state.pkey] = { allOf: [ { $ref: tmpRef } ], description: obj.description };
+                }else{


Spaces around else please.

Your new test case doesn't seem to have an options.yaml file, so I can't see how refSiblings is set of allOf for your test?

I have modified the existing issue130-allOf test case by just extending its openapi.yaml, see https://github.com/Mermade/oas-kit/pull/287/files#diff-afd7c7c1bf17445ff8474a91f25cd72cR18.
By these modifications the if and else branch is tested (lines 310-314).

ideadapt · 2020-10-01T19:31:16Z

I thought, that test cases for other refSiblings values already exist ;). I will wait for your update though. After that I can fix the whitespace issues. Maybe you should consider adding eslint or similar to avoid trivial formatting issues in the future.

MikeRalphson · 2020-10-01T19:38:44Z

eslint is already set up. Travis CI is currently broken as it keeps forgetting my orgs.

ideadapt · 2020-10-01T19:53:38Z

Ok, now I see the .eslintrc :) In this case I propose you activate the rules keyword-spacing and object-curly-newline. I did not do it right now in this PR because it requires to auto fix dozens of lint errors.

MikeRalphson · 2020-10-02T10:49:30Z

In this case I propose you activate the rules keyword-spacing and object-curly-newline. I did not do it right now in this PR because it requires to auto fix dozens of lint errors.

Thanks, I've set keyword-spacing to error and fixed the half-dozen issues. object-curly-newline doesn't match the coding style of this project, nor did it flag any additional issues.

…ext to new allOf (and not into allOf array). Only e.g. an existing properties object (or any other sub schema) next to a $ref property should be moved into allOf array (see also https://json-schema.org/understanding-json-schema/structuring.html#id5). see also Mermade#130

… not active at the moment though).

ideadapt · 2020-10-02T11:32:23Z

In this case I propose you activate the rules keyword-spacing and object-curly-newline. I did not do it right now in this PR because it requires to auto fix dozens of lint errors.

Thanks, I've set keyword-spacing to error and fixed the half-dozen issues. object-curly-newline doesn't match the coding style of this project, nor did it flag any additional issues.

You have now only fixed the lint errors in one files. There are still many eslint errors in the project (most of them space-before-blocks, no-undef and keyword-spacing)

MikeRalphson · 2020-10-02T11:36:07Z

You have now only fixed the lint errors in one files. There are still many eslint errors in the project (most of them space-before-blocks, no-undef and keyword-spacing)

Where are you seeing additional errors, are you runnning npm run lint?

MikeRalphson · 2020-10-02T11:39:59Z

.eslintrc.json

@@ -240,7 +240,7 @@
        "sort-imports": "error",
        "sort-keys": "off",
        "sort-vars": "error",
-        "space-before-blocks": "off",
+        "space-before-blocks": "error",


Please revert this change.

oops. undid it. although these is the rule that in my opinion should be active, because it would have avoided my "}else{" change , "} else {" would be valid.

Yes, eslint's rules are blunt objects and are not IMHO fine-grained enough to express the full range of desired formatting. If you look at the code style of this project, you will see that

if (expression) { // block } else { // block }

is the preferred style.

MikeRalphson · 2020-10-02T20:08:12Z

OK, I don't think we're getting anywhere. The code is working as designed. You can't fix the bugs in other projects (that you don't acknowledge) here.

ideadapt · 2020-10-03T07:42:16Z

Since these other projects work according to spec, I don't think they are buggy. I can give you more examples of other tools, all treating the allOf / $ref problem the same way (not the way oas-kit does).
This whole $ref siblings discussion has been around for some time now in OAS community. In OAS 3.1 e.g. $ref siblings are valid in schema objects, see also OAI/OpenAPI-Specification#556 (comment) .
Also this one nestjs/swagger#978 is interesting, since its very similar to this PR.

I don't see, why my proposed change would make oas-kit worse. In my opinion it would become more useful and catch up with other tools.

MikeRalphson requested changes Oct 1, 2020

View reviewed changes

MikeRalphson added a commit that referenced this pull request Oct 2, 2020

fix: update .eslintrc keyword-spacing rule, refs #287

ab49f52

ideadapt added 3 commits October 2, 2020 13:30

activate eslint rule as error and apply auto fixes.

c5927c1

apply eslint auto fixes related to white spaces (the eslint rules are…

18e7839

… not active at the moment though).

ideadapt force-pushed the fix-allOf-description branch from 9ce4a1a to 18e7839 Compare October 2, 2020 11:32

MikeRalphson reviewed Oct 2, 2020

View reviewed changes

undo accidental rule activation.

3ee82c4

MikeRalphson closed this Oct 2, 2020

ideadapt mentioned this pull request Oct 27, 2020

Fix all of description ideadapt/oas-kit#1

Open

Uh oh!

Fix misplaced description property if refSiblings = allOf #287

Fix misplaced description property if refSiblings = allOf #287

Uh oh!

Conversation

ideadapt commented Sep 30, 2020

Uh oh!

MikeRalphson commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeRalphson Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ideadapt Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ideadapt commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeRalphson commented Oct 1, 2020

Uh oh!

ideadapt commented Oct 1, 2020

Uh oh!

MikeRalphson commented Oct 2, 2020

Uh oh!

ideadapt commented Oct 2, 2020

Uh oh!

MikeRalphson commented Oct 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeRalphson commented Oct 2, 2020

Uh oh!

ideadapt commented Oct 3, 2020

Uh oh!

Uh oh!

MikeRalphson Oct 2, 2020 •

edited

Loading

ideadapt Oct 1, 2020 •

edited

Loading

ideadapt commented Oct 1, 2020 •

edited

Loading