maxdome / swagger-combine

Combines multiple Swagger schemas into one dereferenced schema.
MIT License
132 stars 36 forks source link

include/exclude paths based on provided regex strings #74

Closed tmack8001 closed 5 years ago

tmack8001 commented 5 years ago

I put some thought to having the behavior as described in https://github.com/maxdome/swagger-combine/issues/61 though the issue that I haven't addressed is how to get regex path matching with also matching the method (similar to how the lodash native _.pick functions with objects and subpaths. (eg. /pets/*.get should match all GET operations on /pet prefixed paths, or /.*/{id}.get should match all GET operations on a <resource>ById endpoint).

The code I'm proposing will function as the requester asked (include paths based on regex match) this is also behavior I'm looking at having as well instead of having to include each and every path individually now able to match on whitelists instead.

As a work around the above problem a user can combine regex inclusion with path.method exclusion to get to the subset of paths they want to have in the final combined swagger file.

I've added tests for the tests of including and excluding via regex, @fabsrc let me know what other tests you would like for this new functionality or where else you want to have documentation for this behavior as well as the limitations with it.

tmack8001 commented 5 years ago

@fabsrc any comments on how I have solved this or additional work you would like me to add to get this supported? This pattern of providing regex include/exclude is super helpful for me to include or exclude whitelisted path prefixes (main reason) into our gateway merged swagger definition.

fabsrc commented 5 years ago

Hey @tmack8001. Thanks for your pull request. 🙂 This really is a great addition for this module. Your implementation looks good to me. Thanks for also writing tests and updating the docs.

Do you think it might somehow be possbile to get this filtering with regex by method you mentioned to work as well? It would be nice if something like this: /.*/{id}\.(get|post) would also work.

A possible solution for this could be to generate a list of paths with the methods first, filter those with the regex patterns and then use those filtered "dotpaths" in the _.pick function, e.g.:

const dotPaths = Object.keys(schema.paths)
  .reduce((allDotPaths, currentPath) => {
    allDotPaths.push(currentPath);
    const methods = Object.keys(schema.paths[currentPath]).filter(method => operationTypes.includes(method));
    return allDotPaths.concat(methods.map((method) => `${currentPath}.${method}`))
  }, []);
const matchedDotPaths = dotPaths.filter((dotPath) => this.matchInArray(dotPath, this.apis[idx].paths.include));

Do you want to extend your implementation and try out something like this? If not, it might be helpful to include the limitations of the regex include/exclude in the README.

tmack8001 commented 5 years ago

Thanks for your feedback. I will explore what you have suggested above and give it a try. I do think the main use case of this feature won't care about the method being used and instead be more the generic solution of gateways / proxies doing work on paths (specifically prefixes / namespace pattern).

I'm now to lodash and so a little learning curve on that and I appreciate the suggestions you have provided.

tmack8001 commented 5 years ago

Sorry I let this go stale. I'll look at updating this over the next few days, life and other stuff got in my way.

tmack8001 commented 5 years ago

Got around to updating my approach with your recommendation. I want to DRY this up to use a method reference for the regexExpansion and will push a commit with that contralization today. Added 2 tests for the regex + method one for inclusion and one for exclusion.

Will also update documentation and examples for usage.

tmack8001 commented 5 years ago

@fabsrc the approach has been updated / dry'ed and documented. Let me know if there are other feedback you might have on the approach.