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

Validation fails in RPC context for MqttContext #13350

Closed
2 tasks done
Nosfistis opened this issue Mar 21, 2024 · 6 comments
Closed
2 tasks done

Validation fails in RPC context for MqttContext #13350

Nosfistis opened this issue Mar 21, 2024 · 6 comments
Labels
needs triage This issue has not been looked into type: bug 😭

Comments

@Nosfistis
Copy link
Contributor

Did you read the migration guide?

  • I have read the whole migration guide

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Potential Commit/PR that introduced the regression

#13225

NestJS version

10.3.3 -> 10.3.4

Describe the regression

I use ValidationPipe as a global pipe, even in MQTT microservices.

After the update the MqttContext cannot be successfully validated, so any controller using the @Ctx() context: MqttContext way of accessing the message's topic fails.

I have digged a lot in the release changes, and found that the issue is adding the RouteParamTypes.RAW_BODY in between other types. This causes the key "6" to be exchanged for a RouteParamTypes.PARAM, which applies the type: 'param' to the metadata, instead of type: 'custom'.

Minimum reproduction code

No response

Input code

Expected behavior

The MqttContext passes validation without issues.

Other

No response

@Nosfistis Nosfistis added needs triage This issue has not been looked into type: bug 😭 labels Mar 21, 2024
@kamilmysliwiec
Copy link
Member

cc @tolgap 👀

@rbnayax
Copy link
Contributor

rbnayax commented Mar 21, 2024

Same issue for RPC. RpcParamtype.CONTEXT value 6 used to be mapped to RouteParamtypes.HEADERS which was mapped to custom in ParamsTokenFactory . Now it is mapped to RouteParamtypes.PARAM.
The mentioned commit did introduce this regression but I think the issue here is not related to it, but to the fact that there is some sort of dependency between enum values across the packages which is not correct, and not enforced correctly. @kamilmysliwiec thoughts?

@rbnayax
Copy link
Contributor

rbnayax commented Mar 21, 2024

The type coercion here is the culprit

const token = this.paramsTokenFactory.exchangeEnumForString(
type as any as RouteParamtypes,
);

@tolgap
Copy link
Contributor

tolgap commented Mar 21, 2024

Wasn't aware that the position was this important for this. @kamilmysliwiec do we revert in the meantime then come back to this?

@rbnayax
Copy link
Contributor

rbnayax commented Mar 21, 2024

@tolgap it shouldn't matter, but it was abused :-(

I suggest a diffrent thing:

export enum RpcParamtype {
  PAYLOAD = RouteParamtypes.BODY,
  CONTEXT = RouteParamtypes.HEADERS,
  GRPC_CALL = RouteParamtypes.FILES,
}

Working on a PR now.

@rbnayax
Copy link
Contributor

rbnayax commented Mar 21, 2024

@tolgap opened #13351 this should revert to the original behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into type: bug 😭
Projects
None yet
Development

No branches or pull requests

4 participants