apiaryio / fury-adapter-swagger

Swagger 2.0 parser adapter for Fury.js
MIT License
11 stars 12 forks source link

support root extensions #177

Open abacaphiliac opened 6 years ago

abacaphiliac commented 6 years ago

fix(root-extensions): similar to #174, this will parse root extensions per the swagger specification.

unfortunately, root extensions are parsed into the same location as the info extensions.

abacaphiliac commented 6 years ago

i believe the tests are failing on a swagger zoo assertion, where the root x-ignored extension is now being parsed: https://github.com/apiaryio/swagger-zoo/blob/v2.12.0/fixtures/features/swagger/extensions.yaml#L6

pksunkara commented 6 years ago

Can you please make a PR on swagger-zoo fixing it?

pksunkara commented 6 years ago

You don't need to add a test case here if you fix it there.

abacaphiliac commented 6 years ago

@pksunkara thanks for the advice! i will do that today.

pksunkara commented 6 years ago

Reference to previous discussion: https://github.com/apiaryio/fury-adapter-swagger/pull/71#discussion_r68119552

kylef commented 6 years ago

This would result in multiple extension elements in the API element, the API Elements consumer has no idea which one is which, and it could be really confusing when there is an extension from path, root and info which are using the same name.

For example:

info:
  aws-foo: foo
paths:
  aws-foo: bar
aws-foo: foobar

The parse result will contain an API with three extension elements with different values for aws-foo, as a consumer of the parse result you have no idea which one is which and since it could be the same name it could alter the meaning.

I think we need to find an alternative solution. Perhaps the href for the extension (https://help.apiary.io/profiles/api-elements/vendor-extensions/) could provide more information on the path, for example:

https://help.apiary.io/profiles/api-elements/vendor-extensions/#info
https://help.apiary.io/profiles/api-elements/vendor-extensions/#paths
https://help.apiary.io/profiles/api-elements/vendor-extensions/#root

@abacaphiliac @pksunkara what do you think?

abacaphiliac commented 6 years ago

@kylef i agree with your concern.

vendor-extensions links to element-definitions, which provides the following example:

{
  "element": "extension",
  "meta": {
    "links": {
      "element": "array",
      "content": [
        {
          "element": "link",
          "attributes": {
            "relation": {
              "element": "string",
              "content": "profile"
            },
            "href": {
              "element": "string",
              "content": "http://example.com/extensions/info/"
            }
          }
        }
      ]
    }
  },
  "content": {
    "element": "object",
    "content": [
      {
        "element": "member",
        "content": {
          "key": {
            "element": "string",
            "content": "version"
          },
          "value": {
            "element": "string",
            "content": "1.0"
          }
        }
      }
    ]
  }
}

info, paths, and root seem like fitting relation attributes to me.

edit: i see now that we're already using profile as the sole relation, exactly like the example.

abacaphiliac commented 6 years ago

@kylef href fragment makes sense to me unless there is another more easily parsable location.

are there any other attributes or meta that we can use with the link element? the extension element example uses an object element as content. we could put some kind of source information there, but i think that would conflict with the existing extension element content structure.

pksunkara commented 6 years ago

I like the href fragment idea.

abacaphiliac commented 6 years ago

@kylef @pksunkara thanks for the feedback. i've updated the parser to add URL fragments for info, paths, and root extension types only. it is trivial to add fragments for the other extension types, if consistency is desirable.

swagger-zoo still needs to be updated.

URL fragment is the only mutable detail on the profile link. is that acceptable? i considered accepting a full URL object and merging overrides onto the existing profile link as a base. is that better, or would that be over the top?

pksunkara commented 6 years ago

Code looks good. Can you do a corresponding swagger-zoo PR?

abacaphiliac commented 6 years ago

@pksunkara I'd be happy to open a corresponding swagger-zoo PR.

Would you prefer I hold off on that until the profile and serializer details settle? Is there any way I can contribute to that discussion or project(s)?