-
Notifications
You must be signed in to change notification settings - Fork 4
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
schemas for serialized requests and usage in engine #238
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
Left a comment about global and module pipe transforms.
Add an E2E test with gas to ensure it's working.
return this.evaluationService.evaluate(clientId, body).then((response) => { | ||
return SerializedEvaluationResponse.parse(response) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's step up your game on the controller implementation by using DTO for documentation and serialization.
The way you implemented the DTO today isn't adding much value because Nest doesn't know how to use it.
@Post()
@HttpCode(HttpStatus.OK)
// Let's start by adding Swagger decorators for documentation.
@ApiOperation({
summary: 'Evaluates a request into a decision'
})
@ApiHeader({
name: REQUEST_HEADER_CLIENT_ID
})
@ApiResponse({
status: HttpStatus.OK,
type: SerializedEvaluationResponseDto
})
async evaluate(@ClientId() clientId: string, @Body() body: EvaluationRequestDto) {
const response = await this.evaluationService.evaluate(clientId, body)
// Now let's use the DTO to serialize the response.
return new SerializedEvaluationResponseDto(response)
}
Let me know if that doesn't work
The serialization must be configured by either module and/or application. As good practice, we always add it to the application level, so all modules have it in production.
armory/apps/policy-engine/src/main.ts
Lines 17 to 21 in cdd72ef
const withGlobalPipes = (app: INestApplication): INestApplication => { | |
app.useGlobalPipes(new ValidationPipe({ transform: true })) | |
return app | |
} |
Note the { transform: true }
option.
You'll notice we also add it per module. The reason is that modules are standalone functionalities, and we test them in a mock server instead of the real one.
Since we're moving away from the standard DTOs for validation and serialization, we'll need to use nest-zod utilities.
- Add a new global to ensure we have serialization for Zod
- Add a provider in the modules
{ provide: APP_INTERCEPTOR, useClass: ZodSerializerInterceptor }
(see https://github.com/risen228/nestjs-zod?tab=readme-ov-file#include-zodserializerinterceptor-in-application-root)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the link. I've implemented it following the documentation you linked. Meaning:
- ZodSerializer interceptor set at the module level, not at the whole app level.
- Define ZodSerializer schemas on controller if you want to ensure serialization
As you asked 'let me know if that doesn't work', your code snippet as is didnt. Also, from what I understood of the documentation and what I've tried, you need to specifically add @ZodSerializerDto
decorator with the schema you want to use on every route that uses the interceptor. Did you wanted to pass the SerializerDto as part of the @ApiResponse decorator ? If so, I didn't managed to configure that, nor do I think it is possible looking at the documentation.
export const SerializedEvaluationResponse = EvaluationResponse.extend({ | ||
request: SerializedRequest.optional() | ||
}) | ||
export type SerializedEvaluationResponse = z.infer<typeof SerializedEvaluationResponse> | ||
|
||
export type Hex = `0x${string}` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for refactoring it.
* schemas for serialized requests and usage in engine * Zod interceptor to return correct DTO from controller * added e2e test for serialization
No description provided.