kogosoftwarellc / open-api

A Monorepo of various packages to power OpenAPI in node
MIT License
892 stars 235 forks source link

Overwriting API doc for operation, when multiple operations share the same controller function #778

Closed jjdonov closed 2 years ago

jjdonov commented 2 years ago

769 introduced a regression where multiple operations that share the same controller function now overwrite each others api doc, meaning that the last operation processed is on the only documentation that gets used.

While this feature was undocumented, we were making use of it. Since the framework does not guard against overwriting the api doc, I'm led to believe this was the intended behavior. This worked in previous version of the framework because of the .bind call

I am working on a PR to fix, but could use some direction from maintainers

(from original PR)

It seems like [.bind] is safe to remove, since we inject dependencies later on for all routes

If its not safe, then

  • What test cases should I add to check this behavior?
  • How should bind be applied to an array of middlewares? Would we only bind the last element (the controller)?

I see two options:

  1. Do the .bind and attach the apiDoc to each middleware in the array. This would change the this context for the middlewares which could be problematic in some cases.
  2. Only do the .bind and attach apiDoc to the last middleware in the array, which most of the time should be the operation handler. If someone were to add an after middleware, that may break the implementation. However, after middlewares (outside of error handling middleware) isn't a common pattern in express from what I've seen. I am not sure about the other web frameworks that this library supports.

Thoughts on the above, or ideas for a cleaner solution?


Edit: Here is a (failing) test that demonstrates the issue