feat: localize error in details #1511

jannyHou · 2018-07-11T02:15:08Z

Description

connect to #1489
Original comment from @bajtos

Discussion

LB3 and AJV error details are in different flavours:

LB3: organized by detail attribute like codes, messages, and details are stored per field. e.g.:

// error.details
{
  codes: {
    field1: ['errCode1', 'errCode2'],
    field2: ['errCode1', 'errCode3'],
  },
  messages: {
    field1: ['errMsg1', 'errMsg2'],
    field2: ['errMsg3', 'errMsg4'],
  }
}

AJV: organized per error, each error entry includes detail attributes like field('dataPath'), code('keyword'), message, etc...e.g.:

// the corresponding ajv.errors of example above
[
  {dataPath: 'field1', keyword: 'code1', message: 'msg1', params: {...additionalInfo1}}
  {dataPath: 'field2', keyword: 'code1', message: 'msg3', params: {...additionalInfo3}}
  {dataPath: 'field1', keyword: 'code2', message: 'msg2', params: {...additionalInfo2}}
  {dataPath: 'field2', keyword: 'code3', message: 'msg4', params: {...additionalInfo4}}
]

For the purpose of localizing error in details, either of those 2 flavour seems good to me.
I tried to convert the ajv style to LB3 style and find it's quite complicated, because

dataPath cannot always be used as the id for a field.
- See the first error entry in the following picture: a missing property is easy to localize from message but the dataPath is empty...
I need more time to investigate how many keywords ajv has, and how to parse the field id for each type
- a reference of keywords that ajv uses
- given a quick look there are too many keywords...is it worth to try everything?

@bajtos thought? You may have more insights regarding different signatures.

virkt25

Does it make sense to remove the leading . in the dataPath property ... for example for .isComplete => isComplete.

virkt25 · 2018-07-11T14:36:29Z

packages/rest/test/unit/request-body.validator.test.ts

      {
-        description: 'missing required "title"',
+        description: 'a todo missing required field title',


Shouldn't title be in quotes?

shimks

LGTM

shimks · 2018-07-11T14:31:28Z

packages/rest/src/rest-http-error.ts

-    return new HttpErrors.UnprocessableEntity(msg);
+  export function invalidRequestBody(): HttpErrors.HttpError {
+    return new HttpErrors.UnprocessableEntity(
+      'The request body is invalid. See error object `details` property for more info.',


Shall we extract out the message into a variable like the other functions above?

👍 definitely.

shimks · 2018-07-11T14:38:06Z

packages/rest/test/unit/request-body.validator.test.ts

    validateRequestBody(body, spec, schemas);
+    throw new Error('function `validateRequestBody` should throw error.');
+  } catch (err) {
+    expect(err.message).to.equal(errorMatcher);


we should use shouldjs's throw to assert for properties in the error object

@shimks shoudjs doesn't support a customized error assertion function, that's why I have to take this workaround. I need to do a deep equal check for error.details.

And I also tried do the assertion inside a expect.to.throw, like this:

function shouldThrowError() { try { throwError(); } catch (err) { // assertion 1 expect(err.details).to.deepEqual(expectedDetails); throw err; } } // assertion 2 expect(shouldThrowError).to.throw(expectedMessage)

Its problem is if assertion 1 fails with message1, assertion 2 will receive message 1 instead of the expectedMessage thrown from throwError(), then assertion 2 fails too(but it shouldn't), and prints a very unreadable message, sth like:

// message 1 is a super long error log for a deepEqual assertion message 1 doesn't match expectedMessage

@jannyHou When you say shouldjs does not support customized error assertion, do you mean that it can't take in an additional object with key/value pairs that may exist inside the error object, or that it can't do deep assertions on the properties? Because throw() can take in a second argument with the customized properties: https://shouldjs.github.io/#assertion-throw

I'm trying to push for the throw() so that the code doesn't have to manually throw an error if no error was thrown. I guess it's not a big deal though

IIUC, @jannyHou is saying that Assertion#throw is not performing a deep equality check on error properties, in which case I am fine with the current solution.

@shimks thanks for the doc 👍 will try it if I have time but like Miroslav said, it probably couldn't do a deep equality check. will let you know then.

@shimks even I provide {details: details} in the 2nd input like:

function validate() { validateRequestBody(body, spec, schemas); } expect(validate).to.throw(errorMatcher, { // tests still pass if you change the code to `details: {}` details: details, });

it doesn't do the equality check, but only checks the error message match. Tests still pass if you change the code to details: {}.

I think the correct usage would be a single-arg invoke of throw:

expect(validate).to.throw({ message: /* message text (strict comparison? not ideal) */ details: details });

Please note that {details: details} can be shortened to {details}, and also don't forget to await the expect statement.

await expect(validate).to.throw({details}); // or await expect(validate).to.throw({ message: /*...*/, details, });

I don't really mind implementation details of verifyValidationRejectsInputWithError as long as it works correctly in all cases. As I wrote earlier, the current implementation is fine with me.

jannyHou · 2018-07-11T17:02:34Z

@virkt25 thanks for the feedback:

Does it make sense to remove the leading . in the dataPath property ... for example for .isComplete => isComplete.

I do feel a little bit weird when first saw it...while on a second thought, I think it's a good implication of "a property of object", some type like array data doesn't have that dot, see example in this test
And considering we are supporting more content-types in the future, the request body would not always be an object.

jannyHou · 2018-07-11T18:00:34Z

Based on #1511 (comment), I also have some thought about the naming conversion of ajv error metadata:

dataPath: sounds straightforward enough to me
keyword: I prefer to use code instead...
message: lgtm
params: I prefer to use additionalInfo or sth like it that starts with additional*

Not a strong opinion and let's discuss after agreeing on the signature :).

b-admike

params: I prefer to use additionalInfo or sth like it that starts with additional*

I agree with naming this key additionalInfo.

bajtos

LGTM at high level.

I would like to see the information about details structure from the pull request description captured in our docs eventually (as part of DP3). The docs should mention things that may be surprised to our users, e.g. that required properties have dataPath pointing to the owning object, not the missing property.

I also have some thought about the naming conversion of ajv error metadata:
dataPath: sounds straightforward enough to me

+1

Maybe path would be even better?

keyword: I prefer to use code instead...

+1 to use code

message: lgtm

+1 to keep message

params: I prefer to use additionalInfo or sth like it that starts with additional*

How about just info or data? Not a big deal, additionalInfo is still better to me than params.

bajtos · 2018-07-12T08:47:48Z

packages/rest/src/rest-http-error.ts

  }
+  /**
+   * An invalid request boby error contains a `details` property as the machine-readable error.


typo: boby

bajtos · 2018-07-12T08:48:53Z

packages/rest/src/rest-http-error.ts

  }
+  /**
+   * An invalid request boby error contains a `details` property as the machine-readable error.
+   * Each entry in `error.details` contains the following attributes:


What is the type of error.details - is it an array or an object? It would be great to mention the type here too.

bajtos · 2018-07-12T08:52:35Z

packages/rest/test/unit/request-body.validator.test.ts

    validateRequestBody(body, spec, schemas);
+    throw new Error('function `validateRequestBody` should throw error.');
+  } catch (err) {
+    expect(err.message).to.equal(errorMatcher);


IIUC, @jannyHou is saying that Assertion#throw is not performing a deep equality check on error properties, in which case I am fine with the current solution.

bajtos · 2018-07-12T08:56:42Z

packages/rest/test/unit/request-body.validator.test.ts

    validateRequestBody(body, spec, schemas);
+    throw new Error('function `validateRequestBody` should throw error.');


Let's use a message similar to what shouldjs provides:

throw new Error( 'expected Function { name: 'validateRequestBody' } to throw exception' );

bajtos · 2018-07-12T08:58:32Z

packages/rest/src/rest-http-error.ts

+    dataPath: string;
+    keyword: string;
+    message: string;
+    params: AnyObject;


I am not very happy about using AnyObject from @loopback/repository. My understanding is that AnyObject should be used for values describing plain-data-objects carrying model instance data, e.g. a request body containing Todo data for create method.

Can we use a different type here please? If object does not work, then let's use Object or any.

👌 changed to object

jannyHou · 2018-07-12T13:06:03Z

@b-admike @bajtos thanks for the feedback about naming convention!

How about just info or data? Not a big deal, additionalInfo is still better to me than params.

I eventually go with info since it's concise, better than the longer name additionalInfo IMO.

bajtos

Few more nitpicks to consider, no further review is necessary from my side.

bajtos · 2018-07-12T14:00:04Z

packages/rest/src/validation/request-body.validator.ts

  error.details = _.map(ajv.errors, e => {
-    return _.pick(e, pickedProperties);
+    return {


Nitpick: I think this can be simplified as follows.

error.details = _.map(ajv.errors, e => { path: e.dataPath, // ... info: e.params, });

bajtos · 2018-07-12T14:04:22Z

packages/rest/test/unit/request-body.validator.test.ts

    validateRequestBody(body, spec, schemas);
+    throw new Error('function `validateRequestBody` should throw error.');
+  } catch (err) {
+    expect(err.message).to.equal(errorMatcher);


I think the correct usage would be a single-arg invoke of throw:

expect(validate).to.throw({ message: /* message text (strict comparison? not ideal) */ details: details });

Please note that {details: details} can be shortened to {details}, and also don't forget to await the expect statement.

await expect(validate).to.throw({details}); // or await expect(validate).to.throw({ message: /*...*/, details, });

I don't really mind implementation details of verifyValidationRejectsInputWithError as long as it works correctly in all cases. As I wrote earlier, the current implementation is fine with me.

bajtos · 2018-07-12T14:06:00Z

packages/rest/src/rest-http-error.ts

+   * `ErrorDetails` defines the type of each entry, which is an object.
+   * The type of `error.details` is `ErrorDetails[]`
+   */
+  export type ErrorDetails = {


Since these details are specific to validation errors only (and not to invalidData error for an example), should use a more specific type name, e.g. ValidationErrorDetails? Also use export interface instead of export type!

bajtos · 2018-07-12T14:07:20Z

packages/rest/src/rest-http-error.ts

  }
+  /**
+   * An invalid request body error contains a `details` property as the machine-readable error.
+   * Each entry in `error.details` contains the following attributes:


I think we should move documentation of individual properties into tsdoc entries for each property. Thoughts?

export interface ErrorDetails { /** * A path to the invalid field, e.g. `.name` */ path: string; // ... };

bajtos · 2018-07-12T14:08:12Z

packages/rest/src/rest-http-error.ts

+    path: string;
+    code: string;
+    message: string;
+    info: object;


Is the info (params) object already filled by AJV? Is it possible that AJV can return undefined for certain validation rules? In which case info would need to be marked as an optional property.

ah...params is a required property, while message is not:
from ajv source code:

interface ErrorObject { keyword: string; dataPath: string; schemaPath: string; params: ErrorParameters; // Added to validation errors of propertyNames keyword schema propertyName?: string; // Excluded if messages set to false. message?: string; // These are added with the `verbose` option. schema?: any; parentSchema?: object; data?: any; }

bajtos · 2018-07-12T14:09:21Z

packages/rest/test/unit/request-body.validator.test.ts

@@ -56,8 +68,17 @@ describe('validateRequestBody', () => {
  });

  it('rejects data containing values of a wrong type', () => {
+    const details = [


Please consider adding an explicit type information for details so that the compiler can verify whether the property names below are matching the interface. Same comment applies to other test cases below too.

E.g.

const details: ErrorDetails[] = [ // ... ];

shimks · 2018-07-12T15:10:37Z

packages/rest/test/unit/request-body.validator.test.ts

    validateRequestBody(body, spec, schemas);
+    throw new Error(


We should wrap this error in a variable and assert this in the catch block: expect(err).to.not.eql(noErrorThrown)

@shimks that error doesn't have the same error.message and error.details as the catch block expect to have, so if it happens the 1st assertion expect(err.message).to.equal(errorMatcher); will faill :)

Although I think it's cleaner to directly assert for the error instance created at the moment the validation (unexpectedly) passes, you're right in the sense that the error thrown by AJV is extremely unlikely to have errorMatcher as its message.

Feel free to ignore this comment

bajtos · 2018-07-13T13:42:15Z

packages/rest/src/rest-http-error.ts

+    /**
+     * A humnan readable description of the error.
+     */
+    message?: string;


// Excluded if messages set to false.
message?: string;

IIUC, we don't set messages: false in AJV options, therefore AJV will always fill the message for us and this property is always set.

I am proposing to keep ErrorDetails#message as always provided to make it easier for our clients to handle validation errors. Thoughts?

sounds good reverted it to a required field.

bajtos · 2018-07-13T13:43:48Z

packages/rest/src/rest-http-error.ts

+   * `ErrorDetails` defines the type of each entry, which is an object.
+   * The type of `error.details` is `ErrorDetails[]`.
+   */
+  export interface ErrorDetails {


Re-posting an earlier comment:

Since these details are specific to validation errors only (and not to invalidData error for an example), should we use a more specific type name, e.g. ValidationErrorDetails or perhaps ValidationProblem?

oops sorry I missed that comment, sounds good 👍

shimks · 2018-07-13T15:13:36Z

packages/rest/src/rest-http-error.ts

+     */
+    info: object;
+    /**
+     * A humnan readable description of the error.


jannyHou requested review from bajtos and raymondfeng as code owners July 11, 2018 02:15

jannyHou self-assigned this Jul 11, 2018

raymondfeng approved these changes Jul 11, 2018

View reviewed changes

virkt25 reviewed Jul 11, 2018

View reviewed changes

shimks approved these changes Jul 11, 2018

View reviewed changes

jannyHou mentioned this pull request Jul 11, 2018

Validate request body arguments #1489

Merged

6 tasks

jannyHou force-pushed the localizing-validation-error branch from f4f2113 to e3af709 Compare July 11, 2018 18:40

b-admike approved these changes Jul 11, 2018

View reviewed changes

bajtos approved these changes Jul 12, 2018

View reviewed changes

jannyHou force-pushed the localizing-validation-error branch from da375b3 to 8ca6f49 Compare July 12, 2018 13:02

bajtos approved these changes Jul 12, 2018

View reviewed changes

jannyHou force-pushed the localizing-validation-error branch from 8ca6f49 to 6066291 Compare July 12, 2018 14:38

shimks approved these changes Jul 12, 2018

View reviewed changes

bajtos reviewed Jul 13, 2018

View reviewed changes

shimks reviewed Jul 13, 2018

View reviewed changes

packages/rest/src/rest-http-error.ts Outdated

*/

info: object;

/**

* A humnan readable description of the error.

Copy link

Contributor

shimks Jul 13, 2018

Choose a reason for hiding this comment

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

human

jannyHou force-pushed the localizing-validation-error branch 2 times, most recently from 36d9d70 to 3518a3e Compare July 13, 2018 18:08

feat: localize error in details

d872031

jannyHou force-pushed the localizing-validation-error branch from 3518a3e to d872031 Compare July 13, 2018 19:17

jannyHou merged commit 3c9f6b4 into master Jul 13, 2018

jannyHou deleted the localizing-validation-error branch July 16, 2018 20:09

		validateRequestBody(body, spec, schemas);
		throw new Error('function `validateRequestBody` should throw error.');

feat: localize error in details #1511

feat: localize error in details #1511

Conversation

jannyHou commented Jul 11, 2018 • edited Loading

Description

Discussion

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Jul 11, 2018

jannyHou commented Jul 11, 2018

b-admike left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Jul 12, 2018

bajtos left a comment

Choose a reason for hiding this comment

bajtos Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Jul 11, 2018 •

edited

Loading

jannyHou Jul 11, 2018 •

edited

Loading

bajtos Jul 12, 2018 •

edited

Loading