Skip to content

✨ Adding validations #2285

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

Closed

Conversation

VivekFitkariwala
Copy link
Contributor

Adding validation of policy parameters

@VivekFitkariwala
Copy link
Contributor Author

@bcb37, validations are working. Can you check it once?

Vivek Fitkariwala added 2 commits March 14, 2025 17:32
Adding validation of policy params
@VivekFitkariwala VivekFitkariwala force-pushed the feature/2176-improving-policy-param-validation branch from 386d2c2 to ca11dc1 Compare March 14, 2025 12:02
export class MoocletTSConfigurablePolicyParametersDTO extends MoocletPolicyParametersDTO {
@IsDefined()
@ValidateNested()
@Type(() => Prior)
prior: Prior = new Prior();
prior: Prior;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without the default values for prior and it's properties, the frontend can't create a default object, the priors as "prior": { "success": 1, "failure": 1 } should show up here in the editor.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

The validation isn't quite working for me on the frontend when I fill in prior manually either, it allows me to hit "next" with an object like below. It does catch it on submit and give correct message, however the other validations will raise an error immediately as the user types.

{
"prior": {
"success": "d",
"failure": 2
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not add the default values in the class constructor because class transform uses this class constructor to create the definition and assign the object (to be tested) to the constructed object. If we are adding the default values, the constructor definition also contains the value which we don't want.
For testing, add the default value for Prior in the types
Remove the "prior": { "success": 1, "failure": 1}.
It will not throw an error.

I can add the appropriate config for the frontend side for validation to work correctly.

@bcb37
Copy link
Collaborator

bcb37 commented Mar 14, 2025

@bcb37, validations are working. Can you check it once?

In addition to Dan's comment about the default values (which works if we're defining the types within the root), we also need to disallow extraneous attributes. That works if you add { whitelist: true, forbidNonWhitelisted: true, forbidUnknownValues: true } as the value for 'validate' in the controller.
But we do want to be able to use those default values.

You'll notice that default values are used based on DTO for non-nested attributes (e.g. "batch_size"). I'm still fairly sure the nested types are not being added to the right reflect-metadata instance, though using 'enableImplicitConversion' gets us part of the way there.

@VivekFitkariwala
Copy link
Contributor Author

@bcb37 I don't think that we have two different instances of reflect-metadata because I am able to re-create the issue even after pasting the code in the backend. It seems like it is related to the discriminator.

@danoswaltCL
Copy link
Collaborator

To be continued, validation still need attention but closing as we need the default behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants