cdimascio / express-openapi-validator

🦋 Auto-validates api requests, responses, and securities using ExpressJS and an OpenAPI 3.x specification
MIT License
918 stars 208 forks source link

Manage missing operation handlers #319

Open acrilex1 opened 4 years ago

acrilex1 commented 4 years ago

Is your feature request related to a problem? Please describe. When a request has no handler, the resolver throws a generic Error(). Because I use swagger-codegen to generate a client with a documentation-first workflow, I think that it would be great to be able to declare operationId even when there is still no implementation.

Describe the solution you'd like In defaultResolver(), I would throw NotImplementedException() instead of Error() when no operation handler is found for a route. I would still log the errors, but it would be a better way to handle things and allow requests to continue without returning a 500 error.

If possible, I would also allow the user to include a fallback handler in case none is found.

Describe alternatives you've considered I am currently overriding the operationHandlers' function by calling defaultResolver and catching the thrown error. It works correctly, but I think this should be the default behaviour or at least an option users can use.

I'd be glad to send a PR containing a fix for this issue.

cdimascio commented 4 years ago

@alexandre-okidoo please submit a PR. Thanks!

cdimascio commented 4 years ago

@alexandre-okidoo currently, when a handler function does not exist, the validator will not allow the app to start. it will then output an error message to the console describing the the api whose handler is missing.

one approach to solve this (that doesn't require a code change to the validator) is to define a handler that returns 501 not implemented, then attach this handler to any route that you define in the spec, but do not want to provide an implementation.

would this solve your case?

acrilex1 commented 4 years ago

Yes, this fixed my specific situation, that's what I ended up implementing. However, I had to call the default handler in my handler, which feels a bit sketchy.

Overall, I think this should be available as an option in the config, because as you said, throwing a not implemented http code is a valid behavior and I think that most developer would prefer this behavior over crashing while developing.

cdimascio commented 4 years ago

Would you mind sending me a snippet of what u did?

acrilex1 commented 4 years ago

Hello, here it is:


await new OpenApiValidator({
    ...options,
    operationHandlers: {
      basePath: tmp.name,
      resolver: function (handlersPath: string, route: RouteMetadata) {
        try {
          return defaultResolver(handlersPath, route);
        } catch (e) {
          return (req: Request, res: Response, next: NextFunction) => {
            throw new NotImplementedException(e);
          };
        }
      },
    },
  }).install(router);
cdimascio commented 4 years ago

@acrilex1 , your implementation is perfectly reasonable. @mdwheele implemented "Operation Resolvers" as a means for developers to customize the way operation handlers are resolved. What you've done fits that use case well. (@mdwheele feel free to chime in with your thoughts). That said, it's still worth considering whether Not Implemented (as you have it) should be the default

@acrilex1 , if you're still willing to submit a PR, please do. i'm happy to review it. thanks!

gonenduk commented 3 years ago

I have tried to switch to operation handlers - instead of doing something similar in code. The default revolver verifies that both operation-id and operation-handler exist - or an error is thrown about the missing one. If both exist then the handler function is checked and returned to be declared in a route. And here is my issue: If both are missing, the default resolver simply returns undefined - which causes a 500 server error when trying to set a route to an undefined instead of a function. The error comes from express code and is not very clear. I had to debug to understand what has happened.

2 possible solutions for this can be:

  1. Add a verification in the default resolver and fail if both operation-id and operation-handler are missing.
  2. Continue to return undefined in the default resolver, but skip the route setup which causes the error.

I think option 2 is the best way to go because:

  1. In my case, I have a huge yaml file. I don't want to switch all of it at once to use operation handler, but do it in several steps. Each time I update several routes. The ones I don't update will continue to use the route setup i already have in code. Will be easier to migrate.
  2. When you go design first then code, you don't have the handlers created yet. You can create a draft spec file and put a default "501 not implemented" error. Ignore the 2 properties and put them in the spec once you have the code ready for each route. This way your client side developers can work on your server while it is being written.

Actually, both reason 1 and 2 are the same - they allow a lazy migration to operation handlers. The code change is not in the default resolver but in the installOperationHandlers function.

What do you think?