Skip to content

Commit

Permalink
fix: Change Open API validation middleware to specify and use path pa…
Browse files Browse the repository at this point in the history
…rameters (#8913)

## About the changes
Moved Open API validation handler to the controller layer to reuse on
all services such as project and segments, and also removed unnecessary
middleware at the top level, `app.ts`, and method, `useErrorHandler` in
`openapi-service.ts`.

### Important files

#### Before

<img width="1510" alt="1 Before"
src="https://github.com/user-attachments/assets/96ac245d-92ac-469e-a097-c6c0b78d0def">

Express cant' parse the path parameter because it doesn't be specified
on the `use` method. Therefore, it returns `undefined` as an error
message.

#### After

<img width="1510" alt="2 After"
src="https://github.com/user-attachments/assets/501dae6c-fef5-4e77-94c3-128a9f7210da">

Express can parse the path parameter because I change to specify it on
the controller layer. Accordingly, it returns `test`.
  • Loading branch information
0417taehyun authored Dec 20, 2024
1 parent 682f7e0 commit df9292f
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 20 deletions.
4 changes: 0 additions & 4 deletions src/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,6 @@ export default async function getApp(
// Setup API routes
app.use(`${baseUriPath}/`, new IndexRouter(config, services, db).router);

if (services.openApiService) {
services.openApiService.useErrorHandler(app);
}

if (process.env.NODE_ENV !== 'production') {
app.use(errorHandler());
} else {
Expand Down
13 changes: 13 additions & 0 deletions src/lib/routes/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { type IUnleashConfig, NONE } from '../types';
import { handleErrors } from './util';
import requireContentType from '../middleware/content_type_checker';
import { PermissionError } from '../error';
import { fromOpenApiValidationErrors } from '../error/bad-data-error';
import { storeRequestedRoute } from '../middleware/response-time-metrics';

type IRequestHandler<P = any, ResBody = any, ReqBody = any, ReqQuery = any> = (
Expand Down Expand Up @@ -64,6 +65,16 @@ const checkPrivateProjectPermissions = () => async (req, res, next) => {
return res.status(404).end();
};

const openAPIValidationMiddleware = async (err, req, res, next) => {
if (err?.status && err.validationErrors) {
const apiError = fromOpenApiValidationErrors(req, err.validationErrors);

res.status(apiError.statusCode).json(apiError);
} else {
next(err);
}
};

/**
* Base class for Controllers to standardize binding to express Router.
*
Expand Down Expand Up @@ -114,6 +125,8 @@ export default class Controller {
this.useContentTypeMiddleware(options),
this.useRouteErrorHandler(options.handler.bind(this)),
);

this.app.use(options.path, openAPIValidationMiddleware);
}

get(
Expand Down
16 changes: 0 additions & 16 deletions src/lib/services/openapi-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import type { ApiOperation } from '../openapi/util/api-operation';
import type { Logger } from '../logger';
import { validateSchema } from '../openapi/validate';
import type { IFlagResolver } from '../types';
import { fromOpenApiValidationErrors } from '../error/bad-data-error';

export class OpenApiService {
private readonly config: IUnleashConfig;
Expand Down Expand Up @@ -60,21 +59,6 @@ export class OpenApiService {
});
}

useErrorHandler(app: Express): void {
app.use((err, req, res, next) => {
if (err?.status && err.validationErrors) {
const apiError = fromOpenApiValidationErrors(
req,
err.validationErrors,
);

res.status(apiError.statusCode).json(apiError);
} else {
next(err);
}
});
}

respondWithValidation<T, S = SchemaId>(
status: number,
res: Response<T>,
Expand Down

0 comments on commit df9292f

Please sign in to comment.