Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move api-docs route before fallback handler #1956

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

manuelwedler
Copy link
Contributor

Fixes #1953

The api-docs route was created in the cli file because it uses asynchronous code (loadSwagger). For that reason, it is added after the route not found fallback, which is located in the server constructor. This causes the issue linked above.

To move the api-docs route before the fallback, I changed the server creation method from the constructor to a static async create method. This way we can call asynchronous code in the creation method.

This means instead of calling new Server(...), we need to create the server by await Server.create(...). I did it like this because I think it is the cleaner approach, but this is also a breaking change for anyone using the server module of course.

Changes the server creation method from the constructor to an static async create method, because the creating the api-docs route is asynchronous
Copy link
Member

@kuzdogan kuzdogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH this doesn't feel right. I don't think we require a breaking change how to load the Server module to fix this issue.

Does this line really have to be there?

    // Any request that could not be handled by a route earlier will match this
    this.app.use((req, res, next) => {
      next(new RouteNotFoundError("The requested resource was not found"));
    });

We already had error handlers and they weren't causing this problem. This leads me to think the error is caused by this line being not interpreted as an error handler. Is this really the correct way to throw when no matching routes are found?

Or do we even need that line? If I completely remove it we already get a 404:

image

@kuzdogan
Copy link
Member

Here's also another workaround from Claude:

Response

I see the issue. The problem is that your /api-docs endpoint is being caught by the catch-all route handler that throws a RouteNotFoundError before it can be properly handled. Since you're adding the /api-docs endpoint in the cli.ts file (after the Server class is instantiated), it's not being registered before the catch-all handler.

Here's how you can fix this:

// ... existing code ...

    this.app.use("/", routes);

    // Add a method to register routes after initialization
    public registerRoutes(path: string, router: express.Router) {
      this.app.use(path, router);
    }

    // Any request that could not be handled by a route earlier will match this
    this.app.use((req, res, next) => {
      next(new RouteNotFoundError("The requested resource was not found"));
    });

// ... existing code ...

This adds a new registerRoutes method to the Server class that allows you to register additional routes after the Server has been initialized but before the catch-all 404 handler processes the request.

Then in your cli.ts file, you can use this method to register the Swagger UI route after initializing the server but before starting it:

// In cli.ts where you set up the server
const server = new Server(/* your options */);

// After server initialization but before server.listen()
const swaggerUi = require('swagger-ui-express');
const swaggerDocument = await server.loadSwagger(/* your swagger options */);
const swaggerRouter = express.Router();
swaggerRouter.use('/', swaggerUi.serve, swaggerUi.setup(swaggerDocument));
server.registerRoutes('/api-docs', swaggerRouter);

// Then start the server
await server.listen();

This approach allows you to add the Swagger UI routes after the Server is constructed but ensures they're registered before the catch-all 404 handler processes the request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Sprint - Needs Review
Development

Successfully merging this pull request may close these issues.

Serving the swagger docs is broken
2 participants