Skip to content

Update to Swagger 1.2. Fixes #11, #8, #7. - #14

Merged
raymondfeng merged 24 commits into
strongloop:masterfrom
STRML:feature/swagger1.2
Jul 14, 2014
Merged

Update to Swagger 1.2. Fixes #11, #8, #7.#14
raymondfeng merged 24 commits into
strongloop:masterfrom
STRML:feature/swagger1.2

Conversation

@STRML

@STRML STRML commented Jul 4, 2014

Copy link
Copy Markdown
Member

Changes in this PR:

  • Moved public (swagger UI) out of this module and required swagger-ui as a module in package.json.
  • Added swaggerDistRoot option so users of the API Explorer can override any part of the UI (styles, JS, HTML).
  • Compatibility with Swagger 1.2 (spec) (transition guide)
  • Model schemas are now correctly populated.
  • An accessToken can be provided via the header input.

When this is merged, it should be safe to remove strong-remoting/ext/swagger, the associated test, and app.docs().

Fixes #7, #8, #11.

Samuel Reed added 2 commits July 4, 2014 14:28
Added option `swaggerDistRoot` for specific file overrides.
…module.

Will now correctly return model schemas.

Moved swagger.js tests to this module.
@slnode

slnode commented Jul 4, 2014

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@raymondfeng

Copy link
Copy Markdown
Member

@STRML, thank you for the patch. This is a great improvement to our API explorer experience. I started to play with it and a few issues are fixed at:

https://github.com/strongloop/loopback-explorer/tree/STRML-feature/swagger1.2

The PR also exposes some requirements to strong-remoting. For example, the accepts/returns argument should have a way to reference the declaring model as the type. Let's try to create issues as we go.

- Added array items definition.
- Ignored body when specified via arg.http.source.
@STRML

STRML commented Jul 5, 2014

Copy link
Copy Markdown
Member Author

Ok. I've done a little bit of cleanup for array items. I'll create corresponding issues in strong-remoting and link them back here.

Raymond Feng and others added 3 commits July 5, 2014 10:21
Added comments, api version, and better Swagger 1.2 compat.
Route.accepts & route.returns can now share these translations.
@bajtos

bajtos commented Jul 9, 2014

Copy link
Copy Markdown
Member

@slnode add to whitelist

@raymondfeng

Copy link
Copy Markdown
Member

@STRML We are losing the CSS customization made by loopback-explorer. Is there a way to add it back?

@STRML

STRML commented Jul 9, 2014

Copy link
Copy Markdown
Member Author

@raymondfeng Yes, any loopback-explorer overrides of swagger-ui go in /public. I will find the old customizations & restore them.

@STRML

STRML commented Jul 9, 2014

Copy link
Copy Markdown
Member Author

@raymondfeng I've replaced the old styles.css and supporting files in 45172da. At some point someone should go through it and move the individual overrides out.

@bajtos

bajtos commented Jul 9, 2014

Copy link
Copy Markdown
Member

I did a quick read through the patch, I am impressed, especially about the comments. I'd like to do a more detailed review tomorrow.

There is one idea we had with @ritch connected to the migration of swagger code from strong-remoting to loopback-explorer: change the way how resource descriptors are exposed - use regular express routes instead of building a virtual shared object & shared methods. Reasoning: at the moment, loopback-explorer is modifying the application by adding more remotable objects. This also means that to access swagger descriptors, one has to pass the authentication layer configured for regular models. We had to add a patch to loopback to fix this (see lib/application.js).

@STRML what's your opinion on this idea? Do you agree? Does it make sense to make the change as part of this pull request, or would you rather leave it for later?

@raymondfeng

Copy link
Copy Markdown
Member

@STRML Fantastic! I was able to bring up the new explorer with existing application. The changes are merged into https://github.com/strongloop/loopback-explorer/tree/STRML-feature/swagger1.2. Would you like to rebase from it to avoid future merging conflicts?

@STRML

STRML commented Jul 9, 2014

Copy link
Copy Markdown
Member Author

@bajtos I agree. That should be easy to fit into this patch. I'll update shortly.
@raymondfeng Thanks - will rebase as your commit is good context as to why body is not ignored. I need to reformat the commit messages, however, and will update this PR.

@STRML

STRML commented Jul 9, 2014

Copy link
Copy Markdown
Member Author

@bajtos I've completed that change. It significantly reduces the complexity of the implementation. Review when ready. 👍

This completes the migration of swagger processing from strong-remoting
into loopback-explorer.

Added additional usage instructions to README and additional testing.

This commit introduces a change into where resource descriptors
are hosted. They are no longer hosted under /swagger, but instead
under the same path as the Explorer, wherever that may be.
Generally, the resource listing will be available at
/explorer/resources, and api listings under
/explorer/resources/{modelName}.
Comment thread lib/model-helper.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not entirely true. Types can be specified both by constructors (String) and type names (object).

STRML added 7 commits July 10, 2014 12:07
- Supports all three LDL array type specifications.
- Added model-helper tests.
- translate-keys no longer modifies the incoming object.
- Non-primitive types are now correctly converted into a primitive
  with a `format` attribute.
- `id` properties are not assumed to be required if they are generated.
- Uses model-helper to parse types for swagger.
- Separated returns & accepts hacks.
- Documentation fixes
- TODO add param regex
Not necessary since this can easily be done with an app.use() before
calling loopback-explorer.
Comment thread lib/route-helper.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you misunderstood my comment. The idea was to not modify the route, but return the accepts in the format needed by swagger.

var accepts = routeHelpers.convertAcceptsToSwagger(route, classDef);

var apiDoc = {
  // ...
  parameters: accepts.map(routeHelper.acceptToParameter(route))
  // ...
}

An even better solution may be to call acceptToParameter from hackAcceptsDefinition.

function buildParameters(route) {
  // add ctor arguments
  // filter out computed arguments
  // for each argument: convert the type to Swagger, convert the argument to Swagger parameter
}

// later
var apiDoc = {
  // ...
  parameters: buildParameters(route)
  // ...
}

Anyways, I guess this is rather subjective. If you don't think my proposal will make the code better or you don't think it is worth the effort, I can live with the current version too.

@bajtos

bajtos commented Jul 11, 2014

Copy link
Copy Markdown
Member

I left few more comments, the patch is almost good to be merged. I'd like to test it a little bit before giving it a green light.

@STRML

STRML commented Jul 11, 2014

Copy link
Copy Markdown
Member Author

@bajtos Addressed your comments; should be good to go.

Comment thread package.json Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

loopback-explorer works with loopback 1.x which depends on express 3.x. Can we make it compatible with loopback 1.x & 2.x?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AFAIK express 4.x apps have the same public api as the 3.x apps, i.e. function(req, res), thus this change was not strictly necessary.

@STRML

STRML commented Jul 11, 2014

Copy link
Copy Markdown
Member Author

@raymondfeng Thanks for the comments, fixed.

@bajtos

bajtos commented Jul 14, 2014

Copy link
Copy Markdown
Member

The code LGTM. Unfortunately I don't have time to test the new explorer myself, I'll leave the final sign-off to Raymond.

Thank you @STRML for your contribution, I appreciate the effort you put into this patch and how quickly you addressed our comments.

Comment thread public/lib/loadSwaggerUI.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The authorization key name should be access_token.

@raymondfeng

Copy link
Copy Markdown
Member

@STRML please fix the last issue I raised above. We're good to go after that. Thanks!

@STRML

STRML commented Jul 14, 2014

Copy link
Copy Markdown
Member Author

Good to go, merge when ready.

raymondfeng added a commit that referenced this pull request Jul 14, 2014
@raymondfeng
raymondfeng merged commit cd873e9 into strongloop:master Jul 14, 2014
@STRML

STRML commented Jul 15, 2014

Copy link
Copy Markdown
Member Author

Thanks, everyone, for helping me get this merged and available to all of loopback's users!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should be able to send access token

4 participants