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

fix: assign @NestedValidation error to parent when property is not a class instance #673

Merged

Conversation

ytetsuro
Copy link
Contributor

@ytetsuro ytetsuro commented Aug 5, 2020

Description

Fixes an error when property with @NestedValidation decorator holds primitive value. The error messages is applied to the wrong place on the child. This PR fixes it and places the error correctly on the parent object instead of the child.

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

fixes #679

@ytetsuro ytetsuro changed the title WIP fix: Fixed an error in the structure of the nestedValidation error messages. fix: Fixed an error in the structure of the nestedValidation error messages. Aug 6, 2020
@romainleduc
Copy link

romainleduc commented Aug 6, 2020

Hello, I have exactly the same problem.

When i do that

@InputType()
export class SubCategoryContentInput {
    @Field()
    @Matches(/^[a-zA-Z]{3,25}$/)
    value: string;

    @Field(type => Language)
    lang: Language;
}

@InputType()
export class SubCategoryInput {

    @Field(type => MainCategory)
    public mainCategory: MainCategory;

    @ValidateNested()
    @Field(type => SubCategoryContentInput)
    public content: SubCategoryContentInput;
}

I get this error

"validationErrors": [
            {
              "target": {
                "mainCategory": "announcement",
                "content": {
                  "value": "test",
                  "lang": "fr"
                }
              },
              "value": {
                "value": "test",
                "lang": "fr"
              },
              "property": "content",
              "children": [
                {
                  "value": {
                    "value": "test",
                    "lang": "fr"
                  },
                  "property": "content",
                  "constraints": {
                    "nestedValidation": "nested property content must be either object or array"
                  }
                }
              ]
            }

And when I want create a array like this

    @ValidateNested({ each: true })
    @Field(type => [SubCategoryContentInput])
    public content: SubCategoryContentInput[];

I get this error

"message": "Argument Validation Error",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "addSubCategory"
      ],
      "extensions": {
        "code": "ARGUMENT_VALIDATION_ERROR",
        "exception": {
          "validationErrors": {},

Sorry for my bad English, it's not my language.

@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch from 20d552b to 108c02e Compare August 6, 2020 16:03
Copy link
Member

@NoNameProvided NoNameProvided left a comment

Choose a reason for hiding this comment

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

Please add tests for the test case defined in #679 (Testing valid behavior for both with primitive value and with an invalid class instance.)

@NoNameProvided
Copy link
Member

NoNameProvided commented Aug 7, 2020

Hi! Thanks for your contribution! As I mentioned in the code review please add tests for the example in the linked issue. Something like:

// not valid test just guideance
expect(validationErrors[0].children[1].constraints).toHaveProperty('isString');
expect(validationErrors[1].constraints).toHaveProperty('nestedValidation');

Also if you can rebase again, that would be nice.

@NoNameProvided NoNameProvided added the status: awaiting answer Awaiting answer from the author of issue or PR. label Aug 7, 2020
@NoNameProvided NoNameProvided changed the title fix: Fixed an error in the structure of the nestedValidation error messages. fix: errors from @NestedValidation are assigned to the child instead of the parent when property is not a class instance Aug 7, 2020
@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch from 108c02e to d6d040d Compare August 8, 2020 02:50
@NoNameProvided NoNameProvided removed the status: awaiting answer Awaiting answer from the author of issue or PR. label Aug 8, 2020
@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch from d6d040d to cb5b4dd Compare August 9, 2020 14:40
@ytetsuro
Copy link
Contributor Author

ytetsuro commented Aug 9, 2020

@NoNameProvided

Thanks for the comment.
Fixed.

And, fixes a problem where the stopAtFirstError option would cause two errors, when set nestedValidation.
Presumably, blame is this fix.

@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch 2 times, most recently from 925365d to 4fdb39a Compare August 17, 2020 10:33
@ytetsuro
Copy link
Contributor Author

@NoNameProvided

Sorry to take you out of your busy schedule, but I hope you'll take a re-review at it when you have time.

@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch 2 times, most recently from 240d739 to 59b3ff4 Compare August 25, 2020 00:23
@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch from 9e4ed0e to fca46f7 Compare January 22, 2021 05:01
@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch from b95efbc to 46dbf13 Compare March 23, 2021 07:14
@ytetsuro
Copy link
Contributor Author

@NoNameProvided

Sorry to take you out of your busy schedule, but I hope you'll take a re-review at it when you have time.

@ytetsuro ytetsuro force-pushed the fix/nestedValidatorErrorPoint branch from 46dbf13 to edf9d70 Compare November 14, 2022 04:42
@NoNameProvided NoNameProvided added the flag: BREAKING CHANGE Issues containing a breaking change, requiring a major release. label Nov 20, 2022
@NoNameProvided NoNameProvided changed the title fix: errors from @NestedValidation are assigned to the child instead of the parent when property is not a class instance fix: assign @NestedValidation error to parent when property is not a class instance Nov 20, 2022
@NoNameProvided NoNameProvided merged commit c913e3c into typestack:develop Nov 20, 2022
@NoNameProvided
Copy link
Member

Thanks @ytetsuro!

@ytetsuro
Copy link
Contributor Author

Thanks merged!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
flag: BREAKING CHANGE Issues containing a breaking change, requiring a major release.
3 participants