fix: Change Open API validation middleware to specify and use path parameters

[0417taehyun]

About the changes

Context

I removed idNumberMiddleware and changed to use Open API validation process to validate path parameters in #8734 .

However, Express can't parse path parameters if they don't exist in the path, which is the first argument of the method such as use, hence it is impossible to parse them by using useErrorHandler method like the one below.

    useErrorHandler(app: Express): void {
        // We need to pass a specific parameter such as `/:segmentId` as the first argument of the `use` method.
        // ex. `app.use('/:segmentId/, (err, req, res, next) => { ... }`
        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);
            }
        });
    }

Nevertheless, we can't pass the specific path parameter to useErrorHandler because it is defined on the top level, app.ts file. Consequently, we need to handle it on the controller level, /routs/controller.ts, like the one below.

// Open API validation handler the same as inner logic of `useErrorHandler` method.
const checkOpenAPIValidationError = () => 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);
    }
};

export default class Controller {

    route(options: IRouteOptions): void {
        this.app[options.method](
            options.path,
            storeRequestedRoute,
            checkPermission(options.permission),
            checkPrivateProjectPermissions(),
            this.useContentTypeMiddleware(options),
            this.useRouteErrorHandler(options.handler.bind(this)),
        );

        // Add OpenAPI validation handler with specified path parameter, `options.path`, and now Express can handle them.
        this.app.use(options.path, checkOpenAPIValidationError());
    }
}

I 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

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

Express can parse the path parameter because I change to specify it on the controller layer. Accordingly, it returns test.

Discussion points

• I think we don't need to add tests about this change because there are already some tests of the fromOpenApiValidatoinError.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 20, 2024 0:31am

[vercel]

@0417taehyun is attempting to deploy a commit to the unleash-team Team on Vercel.

A member of the Team first needs to authorize it.

[0417taehyun]

We can reuse this Open API validation handler for all services by using options.path.

Moreover, it helps to add this middleware, Open API validation error handler, automatically while we add a new route by using this route method.

[gastonfournier]

Hi @0417taehyun thanks for the contribution! We'll try to add this to our next week planning and review it.

[0417taehyun]

@gastonfournier

Thanks! Feel free to inform me if I need to change something or discuss about this with your team.

[0417taehyun]

@gastonfournier

Hi, is there any progress of this?

[gastonfournier]

I've tested locally and it works fine, just adding some minor tweaks.

[gastonfournier]

This can easily be removed because useErrorHandler only handles errors identified as coming from openapi

[gastonfournier]

Do you see a problem on having this as a const? It's the same function all the time, we don't have to create new functions for each option path

Suggested change

	const checkOpenAPIValidationError = () => 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);
	}
	};
	const checkOpenAPIValidationError = 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);
	}
	};

[0417taehyun]

@gastonfournier

I changed it to the constant on b4702e2 commit.

I just followed the way of checkPrivateProjectPermissions function definition like the one below but I agreed with your suggestion.

const checkPrivateProjectPermissions = () => async (req, res, next) => {
    if (
        !req.checkPrivateProjectPermissions ||
        (await req.checkPrivateProjectPermissions())
    ) {
        return next();
    }
    return res.status(404).end();
};

[gastonfournier]

Suggested change

	this.app.use(options.path, checkOpenAPIValidationError());
	this.app.use(options.path, checkOpenAPIValidationError);

Based on my previous comment, now just use the constant (we could rename the constant to something more meaningful like openapiErrorValidationMiddleware or something similar)

[0417taehyun]

@gastonfournier

I changed it to the constant on b4702e2 commit and renamed the function on 0b71d03 commit.

[gastonfournier]

Looks good to me! (LGTM!) I'm just double checking some details with my colleagues and will merge then!