Auth all paths

[cziebuhr]

Changes proposed in this pull request:

• Fix auth_all_paths for openapi3
• Code cleanup

[dtkav]

Cool, thanks for this fix @cziebuhr !

I'm curious about the change from get(..., {}) to setdefault(...). Any particular technical reason for this, or just style? I'm happy either way, just want to learn if I missed something.

Can you split your first commit into (1) fix the bug, (2) change the style? It took me longer than it should have to find the bug (not getting securitySchemes in oas3, test not applying to both specs) in the many lines that were changed.

[cziebuhr]

It's hard to split, because the reason it failed is that properties have been saved in AbstractAPI which only worked for swagger 2. security_definitions is the only property which gets accessed outside of add_operation. For the other ones, one could also omit the setdefault lines and use get with default value directly in add_operation. The difference is, that a new dict or list object would get created for each operation when the respective key is missing in the specification instead of sharing them among all operations. Maybe i'm thinking too much in memory optimization.

For long-term, one could argue if it makes sense to have two different classes (swagger2/openapi3) also for the whole spec and not only for each operation.

[dtkav]

It's hard to split, because the reason it failed is that properties have been saved in AbstractAPI which only worked for swagger 2.

Wait, isn't the bug in the title fixed by the following minor change?

_security_schemes = self.specification.get('components', {}).get('securitySchemes', {})
self.security_definitions = self.specification.get('securityDefinitions', _security_schemes)

FWIW I absolutely agree with the refactor, my feedback was just that it was hard find that change, since your PR changes a lot more than what's in the title. Sorry if I am missing something obvious - it's getting late here. Admittedly I haven't tried to pull your diff and split it myself. I'll give that a go tomorrow so I know what I'm talking about.

I really like your idea of making spec classes. Now that raw_spec and specification exist as separate things (specification has been ref-resolved). You could move the setdefaults to the constructor, and use the Spec object to build the correct Operation as well. It would be nice to have as few if version < ... statements around as possible.
Great idea! Maybe I'll do a proof of concept diff tomorrow. 🤔

[cziebuhr]

Wait, isn't the bug in the title fixed by the following minor change?

_security_schemes = self.specification.get('components', {}).get('securitySchemes', {})
self.security_definitions = self.specification.get('securityDefinitions', _security_schemes)

You're right.

If you do a proof of concept: For #686 I also had in mind to move parts of security_decorator into AbstractAPI, because 80% of the code (especially calling the verify_* wrappers) are only depending on the security definition, but not on the security object of each operation. It would save memory to reuse that wrappers. I just wasn't sure on how to cleanly implement it, because the current structure e.g. passes components from oas3 directly to the operation objects. I thought about some kind of preprocessing or caching, but that would have increased complexity. If we can get an object which represents a swagger/openapi specification, we can do better preprocessing or use object oriented methods for accessing parts of the specification instead of passing bunches of dicts around.

[dtkav]

hey @cziebuhr , are you happy to close this?
#712 should have addressed the bug and #713 and #726 include refactor and cleanup.

[cziebuhr]

The only thing which is not handled is the resolver error cleanup, but that's only minor.