ibox

Hi guys,
the link in “consult the Security article” (in Basic method) is not good
-> http://guide.meteor.com/security.md#secret-code

I think it should be
-> http://guide.meteor.com/security.html#secret-code

rlivingston

In working to take advantage of ValidatedMethod, I was disturbed by the repetition of schema validation code that was already present elsewhere in the validate member of the method. A short look at SimpleSchema revealed that the “pick” function should provide the answer to that. I even saw several clues that efforts were made to enable exactly the approach I was imagining with ValidatedMethod and SimpleSchema and I wonder why they weren’t illustrated in the guide. IMO, best practice would never have duplicated schema definitions that could be shared.

Anyway, I set out to utilize pick to reuse my schema and thought that all was going well… until I tested.

I want my “insert” method to require “name”, allow “description” to be undefined, and “visibility” to default to “private” if not specified. This is exactly what my SimpleSchema definition provided for, so I figured all would be good. I set validate to “Groups.schema.pick([‘name’, ‘description’, ‘visibility’]).validator()” and thought all was good. But, the “visibility” option wasn’t defaulting as defined by my Groups schema.

After digging through the docs, I found that the defaultValue is implemented in the clean stage of SimpleSchema processing and that stage isn’t run by validator(). So, my problem isn’t with “pick”, it is with “validator”. Luckily, aldeed added a “clean” option to validator() in December. Turning on “clean” allows the defaultValue to have effect. Awesome.

The problem I now have is that the “clean” stage is too thorough. It is cleaning away undefined keys without giving any error indication to the caller. This can cause a lot of head pounding when a developer has a typo in a key.

So, does anyone know how to get SimpleSchema to validate while both implementing defaultValue and complaining about fields that aren’t present in the schema?

Hopefully, I’ve excerpted enough of my code below to give a flavor of what is happening.

Groups.schema = new SimpleSchema({
  name: {
    type: String,
  },
  description: {
    type: String,
    optional: true,
  },
  visibility: {
    type: String,
    allowedValues: [
      'public',
      'community',
      'private',
    ],
    defaultValue: 'private',
  },
});

export const insert = new ValidatedMethod({
  name: 'groups.insert',
  validate: Groups.schema.pick([
    'name',
    'description',
    'visibility']).validator({ clean: true }),
  run({ name, description, visibility }) {
    const group = {
      name,
      description,
      visibility,
    };
    return Groups.insert(group);
  },
});

// this fails without "{ clean: true }" on validator call above.
groupId = insert._execute(
  { userId: 'test-user' },
  { name: 'goodname', description: 'good description' }
);

// this works with "{ clean: true }", but I don't want it to.
insert._execute(
  { userId: 'test-user' },
  { name: 'goodname', badField: 'no such field' }
);

1 reply

sashko MDG Staff

If you figure out a better way to do it that works well I’d be happy to include it in the guide!

1 reply

rlivingston

Thanks sashko! Of course, better is always subjective and I’ve yet to feel fully confident about this one. After reading what I’ve written below, do you see any downsides to this approach or ways to make it simpler?

Your response prompted me back to one more look and … I think I’m guilty of coding while tired

The SimpleSchema documentation clearly pointed the way in stating “If you want to change any of the default cleaning options, you can pass in those, too.” The “filter” option is the first of the cleaning options and is the key to the problem.

Setting validate to:

  validate: Groups.schema.pick([
    'name',
    'description',
    'visibility']).validator({ clean: true, filter: false }),

behaves as hoped.

So, I think the key to this approach is to always run validator with { clean: true, filter: false } in this context.

In looking at the clean options, I also noted “extendAutoValueContext”, I wish I had seen that a few days ago as it may lead to a solution to another problem I had in server-side testing of a ValidatedMethod in the context of a with Mocha. The _execute put the userId I had defined in my execution context into the Method, but that didn’t make it into the SimpleSchema validation attached to my collection. I was referencing userId in a defaultValue setting in there and it was coming up undefined. Perhaps I should have forwarded this.userId into SimpleSchema using “extendAutoValueContext” as an option on the insert. If so, should this also be illustrated somewhere in the guide? It’s probably not unusual for defaultValue and autoValue functions in SimpleSchema definitions to reference userId.

I probably should be writing a wrapper of some type that takes a SimpleSchema and calls validator with the proper settings for clean, filter, and extendAutoValueContext and returns a function… hmmm.

I’ve thrown some excerpts from my code that should pretty well reveal the pattern I’m now using below.

const GROUP_ID_ONLY = new SimpleSchema({
  groupId: {
    type: String,
    regEx: SimpleSchema.RegEx.Id,
  },
});

export const insert = new ValidatedMethod({
  name: 'groups.insert',
  validate: Groups.schema.pick([
    'name',
    'description',
    'visibility']).validator({ clean: true, filter: false }),
  run({ name, description, visibility }) {
    //...
  },
});

export const archive = new ValidatedMethod({
  name: 'groups.archive',
  validate: GROUP_ID_ONLY.validator({ clean: true, filter: false }),
  run({ groupId }) {
    //...
  },
});

export const rename = new ValidatedMethod({
  name: 'groups.rename',
  validate: new SimpleSchema([
    GROUP_ID_ONLY,
    Groups.schema.pick(['name']),
  ]).validator({ clean: true, filter: false }),
  run({ groupId, name }) {
    //...
  },
});

1 reply

sashko MDG Staff

You can easily write a ValidatedMethod mixin! https://github.com/meteor/validated-method#mixins

1 reply

rlivingston

Exploring.

I like the idea of mixins, but don’t like that they have to be stated in every Method definition. I feel like I’m replacing something that has to be remembered with something else that has to be remembered. The whole point of ValidatedMethod is to hide boilerplate.

I guess I could wrap ValidatedMethod and specify the mixin in the wrapper, but it feels wrong to add another layer of wrapping.

It seems like the ValidatedMethod package should have a method, perhaps “defaultOptions”, that could specify default options including default mixins. Perhaps there was a conversation on this at some point that I could look back at to understand the choice?

1 reply

sashko MDG Staff

Not in every call - in every definition. That’s a very important distinction!

I think the easiest thing to do would be to make your own wrapper for ValidatedMethod, like MyAppMethod that just passes the mixins you always use.

1 reply

rlivingston

Properly chastised and changed

Somewhat related since I’m essentially writing a SimpleSchema mixin, just what is being imagined beyond the schemaMixin example given in the guide when the guide says

It could be nice to have a SimpleSchema mixin which just lets you specify a schema option rather than having to pass a validator function into the validate option.

Just package infrastructure and some type checking maybe? Perhaps the ability to specify an array of SimpleSchema objects that get automatically combined into one validator function as I did once in my example above? Maybe a check that “validate” wasn’t also specified? (unless arrays of validate functions are supported…)

1 reply

sashko MDG Staff

Maybe the code sample is all there is - but I didn’t want to sign up for maintaining another package!

keithnicholas

One thing that keeps coming up in forums and chat rooms is Meteor methods where they want to return the result of a callback.

Nothing is mentioned here, Also, I’m not sure where its explicitly said that you can return a Promise from a Meteor method. Which is what I use.

2 replies