PE-184 Require samples for non hidden ops

[ibolmo]

This exempts operations that are hidden.

[xavdid]

So the idea here is right, but moving the requirement out of the schema itself and into the function means that the readme says sample is no longer required (which shows up only after you run npm run export && npm run docs):

Key	Required	Type	Description
sample	no	object	What does a sample of data look like? Will use resource sample if missing. Required, if the display is not hidden.

To me that reads a little confusingly because I (as a user) would just scan the required column and be confused when this failed.

I wonder if we could go the other way - make it required but pull out errors if the action is hidden? Do the functional constraints run after the regular checks and do they have access to each others' errors?

If we can't do that ^, maybe we make Required bold in the note so it stands out a bit more?

[ibolmo]

Yep, I didn't like that either. I did try something like this (looks like I lost the stash.. so the below is psuedo at this point):

lib/utils/permitNoSamplesIfHidden.js

'use strict';

const _ = require('lodash');

const isRequiredSampleError = ({ message }) => message === 'requires property "sample"';

module.exports = (validate) => (definition, schema) => {
  const result = validate(definition, schema);
  if (_.get(definition, 'display.hidden')) {
    result.errors = result.errors.filter(({message}) => message !== 'require "sample"';
  }
  return result;
};

But the problem is with TriggerSchema. The anyOf: [...]

https://github.com/tdegrunt/jsonschema/blob/9a185ae193e04f090d29004e419aa43cb8097e4e/lib/attribute.js#L77-L104

Validates each, and if any fails then the sub schema fails. Eventually you'll get an error that the sub-schema is not what it would expect.

I tried to bubble up the error, but it started feeling very hacky and frail.

That's why I ended up going with this approach.

[ibolmo]

In general, it's hard to say that the sample is always required. I also tried to update the schema on the fly (e.g. find the BasicOperationSchema and remove the required samples for that definition and then put it back) but again it was a miss.

[xavdid]

Fair. Other thought- doesn't style checks on the server check for this? In that way, it's valid to have no sample, but everything visible will fail the style checks if it doesn't have one.

Past that, I might poke on this tomorrow unless @eliangcs has any other flashes of insight.

[ibolmo]

Ah, interesting idea, but I wonder about that. Doing a zapier validate runs the schema validate and also does server checks? So one could pass (schema) but then the server invalidates?

If so, I think we're still running into the issue of the sample required: no problem you had originally mentioned. This time, though, the validations are not self-contained in the repo.

[xavdid]

Correct, by default validate does a schema validation followed by a style check.

That would sort of make sense. An object isn't invalid if it doesn't have samples, but they're required for public apps and stuff. I think that separates it nicely.

[ibolmo]

Alrighty so it seems the approach then is to just revert the required samples here, but make them required in the server style checks? I'll follow-up on this tomorrow.

[xavdid]

So I spent some time on this today. JSON schema has conditionally required fields but since this is across schemas (sample is in operation and hidden is in field) that won't work.

I didn't read carefully and basically went down a similar rabbit hole you did, trying to filter errors out right at the end. It works on everything except triggers (since they return a generic anyOf error instead of the more specific missing sample), which is a bummer. I got halfway through posting this comment giving up when I thought about (since it's just one level) we could re-validate against polling and hook operations and do basically the same check we do against actions/creates. We can also make resources more pleasant again, allowing parent samples to be used instead of replicating them across get/list/etc.

I came up with the following, which lives in the validate method:

      // handle exceptions to the rules
      const results = v.validate(definition, mainSchema)
      results.errors = results.errors.concat(
        functionalConstraints.run(definition)
      )
      // samples aren't required on hidden operations
      results.errors = results.errors.filter(e => {
        if (e.name !== 'required' && e.argument !== 'sample') {
          return true
        }
        const end = e.property.includes('resource') ? 4 : 3
        const operation = e.property.split('.').slice(1, end).join('.')
        const disp = _.get(definition, `${operation}.display`)
        return !disp.hidden
      })

      // if operations belong to a resource, they can re-use the parent sample
      results.errors = results.errors.filter(e => {
        if (e.name !== 'required' && e.argument !== 'sample') {
          return true
        }
        if (e.property.includes('resource')) {
          const resource = e.property.split('.').slice(1, 2).join('.')
          const sample = _.get(definition, `${resource}.sample`)
          return Boolean(sample && Object.keys(sample).length)
        } else {
          return true
        }
      })

      // filter busted triggers
      // re-validate against each schema
      // if there's exactly 1 issue and it's missing sample, check hidden; possibly filter
      results.errors = results.errors.filter(e => {
        if (e.name !== 'anyOf' && e.property.includes('triggers')) {
          return true
        }

        const opPath = e.property.split('.').slice(1, 3).join('.')
        const triggerOp = _.get(definition, `${opPath}.operation`)
        // if either schema has only one problem, and that problem is sample, we good
        const basicOperationSchemas = schemas.filter(s => s.id === '/BasicPollingOperationSchema' || s.id === '/BasicHookOperationSchema')

        for (let schema of basicOperationSchemas) {
          const res = v.validate(triggerOp, schema)
          if (res.errors.length === 1) {
            const err = res.errors[0]
            if (err.name === 'required' && err.argument === 'sample') {
              if (_.get(definition, `${opPath}.display`).hidden) {
                return false // the only issue is missing sample, but the display is hidden
              }
            }
          }
        }
        return true // real error
      })

It's a first draft, but the idea is we see what errors are coming out and ignore those missing a sample when hidden. The big upside here is that these can be tested with normal examples and antiExamples, so they'll be pretty straightforward to maintain. They're cross-schema, so they'll mess up regular tests. They get their own tests! Past that It's a little weird to put exceptions into the schema, but it's not the worst.

The one big hitch is that we re-validate schema server-side, including the functional constraints. I'm not sure if this is necessary, since pushing an invalid schema will just break your app. If you're tinkering with local installs of our tools in order to get around schema validation, you're only hurting yourself.

How does that sound?

[xavdid]

Separately, this would also still work in style checks. i was having some trouble getting them to trigger, so we might need to double check that anyway.

[ibolmo]

@xavdid ! thanks for checking in on this further. I was about to get started moving this over to style checks. I've looked through the approach and it makes sense. Let me pull it in to the PR.

[ibolmo]

@xavdid I ended up going back to the current approach.. and gave it another try to support resources as well. I think I have something working. Give it a look!

This way the backend validation passes (since samples are not required [at least in schema]). We may want to, however, add backend functional constraints that are similar to the ones in JS. With the current approach, we're able to do this. We'd just need to pass the schema into the functional constraint checker.

[xavdid]

Awesome! I pulled it and ran through and it looks good.

Past that, I'm torn on which way to go for this (functional constraints vs filtering the error list). To help me think through it, I made a quick table:

	Functional Constraints	Filter Errors	Which is better?
Can validate server-side?	Yes (duplicated function)	Yes (duplicated filter of errors)	constraints because it's simpler (but double code is bad either way)^
What happens if function isn't run?	You could re-write your local install of schema to bypass functional checks and ship sample-less apps	We're giving exceptions to the schema, so the server will block deploys	filter
Testable?	Partially in examples, partially a test file	test file	tie
What do docs look like	required is false, but devs will usually get errors anyway	required is true, but we'll wave that requirement in certain conditions	filtering, it's more explicit

So, I think exceptions are the better pattern to develop if we want to hide less complexity. For example, there's nothing in the docs that mention that children and list are mutually exclusive in FieldSchema (since that's a functional constraint). The dev doesn't know anything about it until their app fails to validate, which feels confusing. To reduce confusion, I like the idea of a more strict schema with exceptions, not a lax schema with hidden rules.

That said, I'd love to hear input from @bcooksey and @eliangcs. This isn't a hill I'll die on, so I'm happy to 👍 if others feel strongly.

---

^ I don't like the fact that we duplicate the functional constraints on the server. I understand why we need to if we want to do server validation, but I'd love to see a more cohesive way to do it that doesn't involve adapting our js functions into python.

[xavdid]

I think this gets phrased weird to the end user. Maybe ... requires "sample", because it's not hidden

[xavdid]

Could you wrap the body in {}? Our eslint would yell at you, but apparently it was never set up on this repo. 😁

[xavdid]

curlies here too

[xavdid]

add else {return null} to comply with this rule

[xavdid]

(added some quick review comments that you don't need to fix until we decide on a path forward. figured i'd do it while the code was fresh)

[eliangcs]

@xavdid I read the conversation several times but is still a little bit confused. Questions:

1. Looks like we have two approaches, Functional Constraints and Filter Errors, correct?
2. Does this PR currently implement Functional Constraints approach?
3. When you say Filter Errors, do you mean that we check the required sample on the server?
4. What do you mean by "What happens if function isn't run?" in the comparison table?
5. If the only bad thing about Functional Constraints is the doc saying required is false, can we simply have a script edit the doc after it's generated?

[xavdid]

Sure! Appreciate you reading through this.

1. That's correct. The goal of the PR is to not make samples required on hidden triggers/actions. We can do that by adding a functional constraint that adds to the error list when a non-hidden action lacks a sample. Alternatively, we generate the error list and then filter out errors that were generated by actions that are hidden. The big difference is what's in the schema: with Constraints, sample is not required and with Filter, sample is required.
2. Yes
3. No. See above for description. As for the server, we need to duplicate whatever we do here on the server (see here)
4. This was exploring the hypothetical that we only ran schema.validate on the server, none of the functional constraints or other error filtering. the Constraints approach would be too lax (since the schema technically doesn't require a sample) while Filter would be overly strict (no convenient exceptions), but still valid.
5. That's an interesting approach! I'm not sure how I feel about it. Feels a little strange to separate the docs from the schema further (now they disagree), but maybe it's a workable solution.

[eliangcs]

@xavdid thanks for the clarification! Assuming you have to override schema.validate to do Filter Errors, I'd choose Functional Constraints as the code looks simpler and the use case seems to fit right in the scope of functional constraints. If the only issue is the doc, we can modify buildDocs.js script to support some kind of "doc annotation", allowing us to put an annotation in BasicOperationSchema.js like:

      sample: {
        description:
          'What does a sample of data look like? Will use resource sample if missing. Required, if the display is not hidden.',
        type: 'object',
        minProperties: 1,
        docAnnotations: {
          required: "(**conditionally**, see description)"
        }
      }

Then we would render "no (conditionally, see description)" for the Required column in the doc.

Feels a little strange to separate the docs from the schema further (now they disagree)

I don't see this approach (doc annotation) that way. The annotation comes from the schema, so I think in some sense, the doc and the schema agree.

[xavdid]

Cool! I like that annotation approach.

@ibolmo Olmo you can go ahead and make the tweaks above and merge. I'll add a jira task to do the doc modification stuff