attempt to hoist better errors #62

xavdid · 2019-02-20T23:38:59Z

attempt to hoist the real issue when validation throws a generic "is not one of x,y"

before

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ = 1 =                                                                                                                        │
│     Property │ App.triggers.blah.operation                                                                                   │
│     Message  │ is not any of </BasicPollingOperationSchema>,</BasicHookOperationSchema>                                      │
│     Links    │ https://github.com/zapier/zapier-platform-schema/blob/v7.6.1/docs/build/schema.md#basicpollingoperationschema │
│              │ https://github.com/zapier/zapier-platform-schema/blob/v7.6.1/docs/build/schema.md#basichookoperationschema    │
└──────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

after

┌───────────────────────────────────────────────────────────────┐
│ = 1 =                                                         │
│     Property │ App.triggers.blah.operation.inputFields[0].key │
│     Message  │ does not meet minimum length of 1              │
│     Links    │                                                │
└──────────────┴────────────────────────────────────────────────┘

xavdid · 2019-02-20T23:40:23Z

.eslintrc

@@ -1,7 +1,7 @@
 {
  "extends": "eslint:recommended",
  "parserOptions": {
-    "ecmaVersion": 8
+    "ecmaVersion": 2018


eslint was complaining about

const {x, ...y} = someObj

so I bumped it. Side note, I'd love to move this to use a more standard config. Standard.js is my personal favorite, but the front-end team's, airbnb's, or others are good bets too.

eliangcs

The unreadability of anyOf error messages is something that bugs me for a long time. Thanks for fixing it! I tested locally and found some error messages are still unclear when it comes to anonymous schema. I wonder if it's fixable.

eliangcs · 2019-02-25T07:25:29Z

lib/utils/makeValidator.js

+  if (ambiguousTypes.includes(validationError.name)) {
+    // TODO: make this smarter?
+    // if it's triggerSchema, check for "type"
+    // if it's request/function, check for "source" or "require". if neither are present, validate against request


(Non-blocker) For this TODO, do you think if it's possible to have a generic solution instead of relying on the prior knowledge we have on the app schema? I was thinking we can measure the "relevance" and find out the best match, similar to what python-jsonschema does.

that... is very cool. It's a little weird because it looks like they just de-prioritize the anyOf errors. In that link, STRONG_MATCHES is an empty set, so i'm a little confused at how much value there is there.

And yeah, i was going back and forth here. On the one hand, if we only fish on polling/hook operation schema, we're already seeing a big improvement. doing more is even better, but i'd happily ship with just that.

One idea I had was to recurse down each option (in this case pollingOperation and hookOperation) and then surface whichever path had fewer errors. Basically, the way we do it now, we could send someone down the wrong path if they do have a very malformed string request or something. Very possible I'm overthinking it though.

My other thought is fix polling vs hook explicitly (since that's the most confusing one - it's usually an issue with an input field or something and it doesn't actually matter which of the two we validate against) and then putting a line in for all other cases of anyOf that says "Please file an issue if you see this so we can better cover this case". And then we can build out this section as needed.

what do you think?

eliangcs · 2019-02-25T07:29:18Z

lib/utils/makeValidator.js

+      const namedSchema = validator.schemas[validationError.schema];
+
+      if (!namedSchema) {
+        return processBaseError(validationError, path);


If I do this for a perform:

perform: { url: 'https://example.com', body: 123 }

I see this error:

┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ = 1 = │ │ Property │ App.triggers.movie.operation.perform.body │ │ Message │ is not exactly one from [subschema 0],[subschema 1],[subschema 2],[subschema 3] │ │ Links │ https://github.com/zapier/zapier-platform-schema/blob/v7.6.1/docs/build/schema.md#[subschema 0] │ │ │ https://github.com/zapier/zapier-platform-schema/blob/v7.6.1/docs/build/schema.md#[subschema 1] │ │ │ https://github.com/zapier/zapier-platform-schema/blob/v7.6.1/docs/build/schema.md#[subschema 2] │ │ │ https://github.com/zapier/zapier-platform-schema/blob/v7.6.1/docs/build/schema.md#[subschema 3] │ └──────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────┘

Is it possible to replace [subschema x] with a more readable text?

maybe! i noticed the same thing. I think it's a problem of going one level too far down. I'll jump back in, but my guess is that the error structure is something like

{ argument: ['subschema 0', 'subschema 1', ...], schema: [{type: 'number'}, {type: 'object'}... ] ... }

so we could pull those types out. I agree with the above that we should be careful doing this too coupled to the structure of the schema itself, but some of these edge cases are pretty wild.

xavdid · 2019-02-27T23:46:18Z

Ok @eliangcs I've cleaned it up and done what I can with the links and errors. I don't think it's perfect, but it's a step in the right direction and we'll be able to iterate on it as needed.

xavdid · 2019-02-27T23:47:31Z

Oh and the python implementation was fine, but changed the structure of the errors enough that it wasn't going to be worth the time to redo the cli code to accomodate it. Plus, best_error only returned a single item from the entire validation while there could be multiples across an entire app.

eliangcs

I dug into this a bit. This is truly not an easy problem. Thanks for the effort! There's a couple of edge cases I think we should fix before we merge, but I'm approving this in advance not to block you.

eliangcs · 2019-02-28T06:33:00Z

lib/utils/makeValidator.js

+  const completePath = makePath(path, err.property).replace(
+    /\.instance\./g,
+    '.'
+  );


I see there are some cases where the path ends with .instance. Maybe we can change it to something like:

const completePath = _.trimEnd( makePath(path, err.property).replace(/\.instance($|\.)/g, '.'), '.' );

yeah, good call. If you've still got it, it would be great to get an input + schema that generated that so I can write a test against it

One example is when you validate a primitive value against FieldSchema. Like inputFields: [123].

eliangcs · 2019-02-28T06:47:56Z

lib/utils/makeValidator.js

+        throw new Error(
+          'If you see this, please file an issue at https://github.com/zapier/zapier-platform-cli/issues'
+        );
+      }


Throwing a hard error here blocks the dev from continuing their work, which is worse than the original behavior IMO. Maybe we want to take a milder approach here. Can we print a warning message and keep validation going, so the dev can see the validation errors at least?

I found one of the reasons why res.errors is an empty array below is because of this jsonschema bug, where validationError.instance is undefined when the value being validated is falsy. To reproduce, try using a falsy value like an empty string in inputFields:

{ inputFields: [''], }

To work around this bug, I suggest we do this before we do validator.validate(validationError.instance, nextSchema):

if (validationError.instance === undefined) { // Work around a jsonschema bug: When the value being validated is a // falsy, such as false and '', validationError.instance isn't available. // See https://github.com/tdegrunt/jsonschema/issues/263 const fullPath = path.replace(/^instance\./, '') + validationError.property.replace(/^instance\./, '.'); validationError.instance = _.get(definition, fullPath); }

Important: You need to pass definition to cleanErrors if you want to use this code.

ha, that's why I made this change but didn't connect the dots. Great find!

The reason I was throwing the error was that there should be an error here (since validation has failed) but we have no error to throw. If we don't throw, we fail to validate but there's nothing to show, which I think is worse behavior. Given that, do you think it's appropriate to throw an error?

I do like pulling the instance out from above though, if we can.

I was thinking like what if they have other errors? So for example, if I do inputFields: [false] and another violation like a trigger description being too short, this would just show me this block of red text:

I'd have no idea what's wrong with my code if I've ever seen this. However, if we print a warning message instead of throwing an error over this, we can still see the other error:

All that is to say even if validation has failed on one branch, we can still keep running validation on the other branches. Does that make sense?

ah, sure. I guess in my head we could return an error instead of throwing it because we don't want to actually halt execution. good call!

eliangcs · 2019-02-28T06:51:54Z

test/readability.js

  });

  it('should have decent messages for minimum length not met', () => {
-    const results = CreateSchema.validate({
+    const results = TriggerSchema.validate({


Any reasons why this change?

Triggerschema operation is itself an anyOf (polling/hook) so I wanted to make sure errors in a nested field (like inputData) correctly surfaces in that situation. Sort of the base case we're trying to handle

attempt to hoist better errors

118f9b3

xavdid commented Feb 20, 2019

View reviewed changes

xavdid added 2 commits February 22, 2019 15:10

fix some tests

c85e1b3

add tests

eb8e787

xavdid marked this pull request as ready for review February 23, 2019 04:06

xavdid requested a review from eliangcs February 23, 2019 04:06

eliangcs reviewed Feb 25, 2019

View reviewed changes

improve how results are returned

3159da3

eliangcs approved these changes Feb 28, 2019

View reviewed changes

fix falsy instances

56ec536

xavdid merged commit 80ecfc2 into master Mar 1, 2019

xavdid deleted the better-schema-errors branch March 1, 2019 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

attempt to hoist better errors #62

attempt to hoist better errors #62

xavdid commented Feb 20, 2019 •

edited

Loading

xavdid Feb 20, 2019

eliangcs left a comment •

edited

Loading

eliangcs Feb 25, 2019

xavdid Feb 26, 2019

xavdid Feb 26, 2019

eliangcs Feb 25, 2019

xavdid Feb 26, 2019

xavdid commented Feb 27, 2019

xavdid commented Feb 27, 2019

eliangcs left a comment •

edited

Loading

eliangcs Feb 28, 2019

xavdid Feb 28, 2019

eliangcs Feb 28, 2019

eliangcs Feb 28, 2019

xavdid Feb 28, 2019

eliangcs Feb 28, 2019 •

edited

Loading

xavdid Feb 28, 2019

eliangcs Feb 28, 2019

xavdid Feb 28, 2019

attempt to hoist better errors #62

attempt to hoist better errors #62

Conversation

xavdid commented Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

eliangcs left a comment • 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

xavdid commented Feb 27, 2019

xavdid commented Feb 27, 2019

eliangcs left a comment • 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

eliangcs Feb 28, 2019 • 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

xavdid commented Feb 20, 2019 •

edited

Loading

eliangcs left a comment •

edited

Loading

eliangcs left a comment •

edited

Loading

eliangcs Feb 28, 2019 •

edited

Loading