validation.js breaks deep object validation that SimpleSchema supports

[nolandg]

SimpleSchema supports deep object validation, eg:

  data: {
    type: Object,
    optional: false,
    viewableBy: ['members'],
    },
  },
  'data.value': {
    type: Number,
    optional: false,
  },

is perfectly valid and when it is validated in step 5: https://github.com/VulcanJS/Vulcan/blob/0c06dc9b55e6bb456124f38f881aebad917c8025/packages/vulcan-lib/lib/modules/validation.js#L59 it passes the SimpleSchema tests fine. But it does not pass the extra Vulcan step 4: https://github.com/VulcanJS/Vulcan/blob/0c06dc9b55e6bb456124f38f881aebad917c8025/packages/vulcan-lib/lib/modules/validation.js#L45 because the special Vulcan validation does not support deep keys. It's trying to do this: document["data.value"] instead of document.data.value

Why does the comment say "run SS validation for now for backwards compatibility"? Aren't we full on using SimpleSchema?

If we just remove Step 4, it works great. SimpleSchema already checks for missing required fields just fine and does it deeply.

Do you want a PR to remove Step 4?

[SachaG]

The goal was to remove SS eventually to have more control over how errors are displayed, and also because SS does a lot of things we don't need. I'm not sure what to replace SS with though when it comes to validation, so for now let's keep it. Maybe we can fix step 4 instead?

[nolandg]

I personally like SS and don't see a reason to duplicate all that functionality. Seems to work great for me.

I'm happy to fix step 4 but do you want to duplicate the checking for missing values there? or just use SS? if the latter, we'll be checking the same thing twice but I guess making it easier to remove SS later.

Was there some talk about Mongo doing some nice document validation built in? That doesn't help much I guess on the client and/or if people want to have the option of other DBs. If anyone has a better idea than SS, I can look into it. What specifically is wrong with it other than doing more than we need...a bit...actually I like most of what it does and can see myself using it.

[SachaG]

I don't have any huge issues with SS, the main ones were that it was heavy and had a handlebars dependency. But that might've been fixed in recent versions. Also, I'm not sure how it deals with internationalizing its error messages. And I find it generally a bit opaque and hard to work with, after all it does a lot of things we might not need.

So nothing unfixable, I just thought it was better to go towards a lighter, more focused solution rather than keep a fairly large, black box codebase (ironic I know since that could apply to Vulcan itself equally well!).

See also: https://github.com/VulcanJS/Vulcan/issues/1903

[nolandg]

cool. so whatdya want?

a. Remove step 4 and let SS do what it's good at or b. Fix step 4 and check for missing fields twice. "Requires" lodash for deep object get.

[SachaG]

b. for now, we can always remove Step 4 later if we decide to.

[nolandg]

Sorry, after attempting this I'm finding it's too much work and I'm not doing it. There is too much logic to duplicate that SS handles.

For example SS supports this case: billingAddress object is optional but if it does exist or if it's being $set then all of it's required child properties like billingAddress.city are required. Now you have to start stepping through the tree one by one, not just blindly checking existence. That's too much to duplicate for me.

And arrays. An array can be optional but if it exists, child properties on it's object elements can be required.

[SachaG]

In that case you could maybe just disable step 4 locally in your own fork? Or maybe even add a settings check? It's a bit hacky but it'd take care of the issue for now.

[nolandg]

Yep. I've blown it away here.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[eric-burel]

Closing as stale, we have updated this part of the code.