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

Fixing the GRPC toError() bug with ErrorValidation #331

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

emmacasolin
Copy link
Contributor

@emmacasolin emmacasolin commented Feb 14, 2022

Description

There is currently a bug with the new ErrorValidation, which was brought in as part of the new validation system in #321. Since ErrorValidation extends ErrorPolykey, but also uses a different constructor, toError() is unable to properly construct ErrorValidations when throwing errors. Since we want ErrorValidation to be able to be used as both a standard ErrorPolykey, but also as a way of joining together multiple ErrorParses into a single error, we need to do something like this:

class ErrorValidation extends ErrorPolykey {
  description = '...';
  exitCode = sysexits.DATAERR;
  public errors: Array<ErrorParse>;

  constructor(message, data) {

    if (data.errors != null) {
      let errors = [];
      for (const eData of data.errors) {
        const errorParse = new ErrorParse(eData.message);
        errorParse.keyPath = ...eData.keyPath;
        errors.push(errorParse);
      }
      this.errors = errors;
    }


  }

  static createFromErrors(errors: Array<ErrorParse>): ErrorValidation {
    const message = errors.map((e) => e.message).join('; ');
    const data = {
      errors: errors.map((e) => ({
        message: e.message,
        keyPath: e.keyPath,
        value: e.value.valueOf(),
      })),
    };
    const e = new ErrorValidation(message, data);
    e.errors = errors;
    return e;
    // super(message, data);
    // this.description = 'Input data failed validation';
    // this.exitCode = sysexits.DATAERR;
    // this.errors = errors;
  }
}

Issues Fixed

Tasks

  1. Fix the constructor inside ErrorValidation to match ErrorPolykey
  2. Add a static method to ErrorValidation to deconstruct an array of ErrorParses
  3. Tests for both usages of ErrorValidation, including with toError()

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@emmacasolin
Copy link
Contributor Author

The bug has been fixed, however there aren't tests for the majority of GRPC handlers that validation errors are handled correctly. This could be added either in this PR or in #311.

@emmacasolin emmacasolin force-pushed the validation-grpc-error branch from d5fc954 to d05d4e5 Compare February 14, 2022 03:49
@emmacasolin emmacasolin marked this pull request as ready for review February 14, 2022 03:51
@emmacasolin
Copy link
Contributor Author

Lintfixed + all tests passing. I added some tests to check that toError() is working correctly with some of the handlers that already have split tests on master. The rest can wait until #311. This is now ready for review/merge.

@CMCDragonkai
Copy link
Member

This looks good, the only thing is to make the commit message more descriptive. State what you are actually fixing in this case:

Fixing `ErrorValidation` serialisation over GRPC by putting `ErrorParse` data into `ErrorValidation.data.errors`

Would be more valid..

@CMCDragonkai
Copy link
Member

After you change the message, proceed to merge.

…se` data into `ErrorValidation.data.errors`
@emmacasolin emmacasolin force-pushed the validation-grpc-error branch from d05d4e5 to 79b50e7 Compare February 14, 2022 05:05
@emmacasolin
Copy link
Contributor Author

Changed the commit name - merging now.

@emmacasolin emmacasolin merged commit ae21266 into master Feb 14, 2022
@emmacasolin emmacasolin deleted the validation-grpc-error branch February 14, 2022 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants