Skip to content
This repository has been archived by the owner on Jul 2, 2019. It is now read-only.

support root extensions #177

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

support root extensions #177

wants to merge 2 commits into from

Conversation

abacaphiliac
Copy link
Contributor

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.

…s per the swagger specification. root extensions are parsed into the same location as the info extensions.
@abacaphiliac
Copy link
Contributor Author

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
Copy link
Contributor

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

@pksunkara
Copy link
Contributor

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

@abacaphiliac
Copy link
Contributor Author

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

@pksunkara
Copy link
Contributor

Reference to previous discussion: #71 (comment)

@kylef
Copy link
Member

kylef commented May 15, 2018

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
Copy link
Contributor Author

abacaphiliac commented May 15, 2018

@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
Copy link
Contributor Author

@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
Copy link
Contributor

I like the href fragment idea.

…profile link URL fragment and added paths extension assertion as well
@abacaphiliac
Copy link
Contributor Author

@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
Copy link
Contributor

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


this.handleExternalDocs(this.api, swagger.externalDocs);

this.validateProduces(this.swagger.produces);
this.validateConsumes(this.swagger.consumes);

const complete = () => {
this.handleSwaggerVendorExtensions(this.api, swagger.paths);
this.handleSwaggerVendorExtensions(this.api, swagger.paths, 'paths');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a breaking change for tooling looking for these relations so we need to be careful about rolling out this change. @pksunkara perhaps we should serialise them differently in minim 1.0/0.6 serialisers?

We have a task in our backlog about moving all of the links (such as those in annotations) to be in apielements.org (since many of them are actually 404s). What we could do it update them all and create a profile registry over in API elements. Then in the minim 0.6 serialiser we can replace them to be the existing links.

Another thing to think about is I don't think the current profile is correct given the API Elements 1.0 serialisation rules. Since we're placing an un-refracted object directly in the content of the extension which breaks the rules. Given the following extension:

x-extension:
  element: string

Tooling would think that this is a string element instead a user-defined object which is some of the problems we was trying to solve in API Elements 1.0. @pksunkara Would love your thoughts, we can probably use this time to do the profile right before wide-adoption of API Elements 1.0.

Copy link
Member

@kylef kylef May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative idea, we could also include both the old style and new style extension element. This allows existing tooling to continue to work (we would have to think of a deprecation plan, a way to communicate that the element is deprecated and a time period in which we support the existing element). This allows new tooling to directly utilise the newer format, and old tooling to utilise the existing format until updated. Personally I think my other comment would be better idea long-term for this particular case given API Elements immaturity, but it involves more work short-term (changing minim serialiser and releasing another toolchain version), but it would be less work in the long run than having to maintain older code paths, but it would be a good idea to start thinking about the way forward for deprecating and replacing elements for the future.

Copy link
Contributor

@pksunkara pksunkara May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your first comment on this.

  • Move the profiles to apielements.org
  • Write an RFC about the content of the profiles and implement it
  • Convert it into 0.6 serialization.

If you want to discuss about the problems we need to fix before 1.0, RFC would be a better place to discuss this.

@abacaphiliac
Copy link
Contributor Author

@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)?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants