For discussion: Support spec-level x-modules, and fix modules at API level #56
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deterministically resolve conflicts & add specific keys before wildcards.
Without this patch, a path combination like
/{something}/else
/robots.txt
was not accepted, as v8 happens to preserve the order of object keys in this
case, and the wildcard would be added before the fixed value. This would cause
the router to enter the wildcard path when trying to add robots.txt.
With this patch, more specific paths (without wildcards) are built first,
equivalent to:
/robots.txt
/{something}/else
gwicke
changed the title
|
/cc @wikimedia/services, for discussion. |
| return P.each(Object.keys(paths), function(pathPattern) { | ||
| // while building the tree. Also sort(), so that fixed path segments sort | ||
| // before patterns (by virtue of `[a-zA-Z/] < '{'`). | ||
| return P.each(Object.keys(paths).sort(), function(pathPattern) { |
There was a problem hiding this comment.
Hm, let's sort a copy like we've discussed in the other PR?
|
I agree this is most like what needs to be done, I've been experimenting with the same stuff yesterday. Although, this code is some complex that I can't really say if this would have any weird implications or not, so I suggest testing RB and Change-Prop with this. |
| @@ -340,6 +340,7 @@ Router.prototype._registerMethods = function(node, pathspec, scope) { | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
|
This depends on #55, so let's settle that one first. |
|
I undid the sorting, as it exposed some latent issues in how we handle path overlaps that weren't easy to fix. For now, we are relying on manually specifying end points in an order that avoids the bugs. I added a FIXME about this issue at the relevant location in the code. |
- Support x-modules at spec level (same level as paths). This allows modules
to define top-level entry points, like /somepath. The alternative would be
to support paths: { '': { 'x-modules': {...} } }, but imho that's uglier
than x-modules at the path level. The swagger spec requires paths to start
with '/' as well, so this would not be a valid swagger spec.
- Preserve operations & some other scope data for API-level modules.
Otherwise, modules at API level do not have access to their own operations.
We have some latent issues in the handling of overlapping paths (such as
/foo and /foo{/bar}) that aren't easy to resolve. For now, we are relying on
speccing entry points in just the right order to work around this.
This patch adds a small, private extension that lets us hide private entry points from the docs by adding an `x-hidden` boolean property at the method level. This is occasionally useful for "boilerplate" entry points like /robots.txt. There are discussions about more advanced ACL-based doc hiding at OAI/OpenAPI-Specification#433, but those haven't lead anywhere yet. This is a simple solution that will work in the meantime.
1 similar comment
| var xModules = spec['x-modules']; | ||
| if (xModules) { | ||
| loadPromise = loadPromise | ||
| .then(function() { |
There was a problem hiding this comment.
Indentation is weird here
|
One nit inlined, other that that GTG |
|
Ok, I'm merging this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
to define top-level entry points, like /somepath. The alternative would be
to support paths: { '': { 'x-modules': {...} } }, but imho that's uglier
than x-modules at the path level. The swagger spec requires paths to start
with '/' as well, so this would not be a valid swagger spec.
Otherwise, modules at API level do not have access to their own operations.