search

krakenjs / swaggerize-routes

Swagger document driven route builder.
Other
58 stars 57 forks source link

Fallback security to root api if missing in verb or path specification #62

Closed jishi closed 7 years ago

jishi commented 8 years ago

According to the swagger documentation, you can have a top-level security declaration that would be applied to all routes, but swaggerize-routes does not take that into consideration.

It only inherits security from a path specification, if it hasn't been declared at a verb declaration.

I couldn't find any tests for the fallback on path, so I wasn't sure where I should add a fallback test for top-level security in this case.

subeeshcbabu-zz commented 8 years ago

LGTM. To add a unit test for this you can add the a sample api here - https://github.com/krakenjs/swaggerize-routes/tree/master/test/fixtures/defs

jishi commented 8 years ago

Hm, you are actually testing: t.ok(route.hasOwnProperty('security'), 'has security property.');

But route always has a security key, but it is undefined, hence, the test passes... which seems broken.

I changed all tests to do t.notEqual(route.xxx, undefined) instead, but then 4 properties in the build with root path test failed. Looking into that now.

jishi commented 8 years ago

Okay, I figured that the tests that broke were faulty, please let me know if this seems sensible.

subeeshcbabu-zz commented 8 years ago

👍

jishi commented 7 years ago

Is this still relevant or has this already been fixed? Should I update this branch? Will you merge it then?

subeeshcbabu-zz commented 7 years ago

I have added this change on the PR for the 2.x branch - https://github.com/krakenjs/swaggerize-routes/pull/72/files#diff-d3fa4da5edb78c3f738e372d08f86ed8R41. The 2.x major version is still in-progress, and may take some time to finally get released.

If you want to get this change merged on the 1.x version, please rebase this PR, I can merge.

jishi commented 7 years ago

Sorry, I've missed your comment. I have updated this fork with the latest changes from master. Will this do?