Repetitive code. How is this package intended to be used?

[elie222]

Hi,

I've started to use this package a bit, and I'm noticing that my code is getting quite repetitive when it comes to validating arguments, and I could just be using simple-schema/collection2 to do all this validation.

As an example, I just created Meteor methods for insert, edit and remove. Each time, I validate all the arguments I receive. But I could have just done this once when defining a schema for the collection. I feel like I'm not using this package in the best possible way at the moment and it's causing me to write repetitive. What are your thoughts on this? How should code be structured with this package and how should it work with an already existing simple-schema?

[stubailo]

Hi @elie222,

Thanks for reporting your concerns with the package! I want to make sure it works for everyone. Can you tell me where your code is repetitive? I think it's natural to have one schema for your collection, and then a new schema for every method. If you find that a lot of your methods have the same schema for the arguments, you can factor it out into a new variable and re-use it. It would be easier to understand the issue if you could post a code sample.

There's a good reason for validating the Method arguments in addition to validating the data when it enters the database. Collection2 does help you ensure that the database has the right schema, but it doesn't do anything to make sure that the database write is logically correct. It's easy to write code where you don't validate everything you think you are validating, and end up with some inconsistent data, even though it matches the schema correctly. This is why it is helpful to tightly restrict all of the inputs to the method itself, so that you can write tests for all of the inputs and make sure they do what you expect.

[elie222]

Here's the code I started writing:

Manager = class {
  constructor() {}

  addAdvert(advert) {
    return addAdvert.call(advert, (e, r) => {
      console.log(e);
    });
  }

  editAdvert(currentName, advert) {
    return editAdvert.call({currentName: currentName, advert: advert}, (e, r) => {
      console.log(e);
    });
  }

  removeAdvert(name) {
    return removeAdvert.call({ name: name }, (e, r) => {
      console.log(e);
    });
  }

  getAllAdverts() {
    return AdManagerAdverts.find();
  }

  // client only
  subscribe() {
    return Meteor.subscribe('adManagerAds');
  }
};

// helper function
function checkIsAdmin(userId) {
  if (!Roles.userIsInRole(userId, ['admin']))
    throw new Meteor.Error('not-admin', 'You must be admin to call this method');  
}

const addAdvert = new ValidatedMethod({
  name: 'AdManager.methods.addAdvert',

  validate: new SimpleSchema({
    name: { type: String },
    url: { type: String, regEx: SimpleSchema.RegEx.Url },
    imageUrl: { type: String, optional: true, regEx: SimpleSchema.RegEx.Url },
    extraData: { type: String, optional: true },
  }).validator(),

  run({ name, url, imageUrl, extraData }) {
    checkIsAdmin(this.userId);

    if (AdManagerAdverts.findOne({name: name}))
      throw new Meteor.Error('already-exists', 'An advert with this name already exists');      

    return AdManagerAdverts.insert({
      name: name,
      url: url,
      imageUrl: imageUrl,
      extraData: extraData,
    });
  }
});

const editAdvert = new ValidatedMethod({
  name: 'AdManager.methods.editAdvert',

  validate: new SimpleSchema({
    currentName: { type: String },
    advert: { type: Object },
    'advert.name': { type: String, optional: true },
    'advert.url': { type: String, optional: true, regEx: SimpleSchema.RegEx.Url },
    'advert.imageUrl': { type: String, optional: true, regEx: SimpleSchema.RegEx.Url },
    'advert.extraData': { type: String, optional: true },
  }).validator(),

  run({ currentName, advert }) {
    checkIsAdmin(this.userId);

    // if we're changing the name, make sure an ad with this name doesn't already exist
    if (currentName != advert.name && AdManagerAdverts.findOne({name: advert.name}))
      throw new Meteor.Error('already-exists', 'An advert with this name already exists');      

    return AdManagerAdverts.update({ name: currentName }, {
      $set: advert
    });
  }
});

const removeAdvert = new ValidatedMethod({
  name: 'AdManager.methods.removeAdvert',

  validate: new SimpleSchema({
    name: { type: String }
  }).validator(),

  run({ name }) {
    checkIsAdmin(this.userId);

    return AdManagerAdverts.remove({ name: name });
  }
});

I don't actually have a simple schema defined, but if I did, it would look very similar to the schemas I'm using for each method. I could collect the validation for addAdvert and editAdvert into a single variable.

I agree that Meteor methods is the way to go, and allow and deny shouldn't be used. I suppose the old way of doing things was very similar. I'd write check for each argument at the start of each method, then do whatever logic had to be done, then insert, and then simple-schema would go and validate all the info again. Just sticks out a bit more now that this package is using simple-schema instead of check to validate. Makes the code duplication stick out.

[elie222]

Btw, I just saw this recent pull request: https://github.com/meteor/validated-method/pull/17

You can see I ran into the same problem with the editAdvert method.

[stubailo]

I'm going to take a few minutes to look at this. In the meanwhile:

You can see I ran into the same problem with the editAdvert method.

Is the solution you have satisfactory? I guess it would be nicer if the SimpleSchema syntax for nested objects was better, ideally you could do:

validate: new SimpleSchema({
  currentName: { type: String },
  advert: {
    name: { type: String, optional: true },
    url: { type: String, optional: true, regEx: SimpleSchema.RegEx.Url },
    imageUrl: { type: String, optional: true, regEx: SimpleSchema.RegEx.Url },
    extraData: { type: String, optional: true },
  }
}).validator(),

[elie222]

That would be a nicer syntax. Not the end of the world the current syntax. On 8 Dec 2015 20:23, "Sashko Stubailo" notifications@github.com wrote:

I'm going to take a few minutes to look at this. In the meanwhile:

You can see I ran into the same problem with the editAdvert method.

Is the solution you have satisfactory? I guess it would be nicer if the SimpleSchema syntax for nested objects was better, ideally you could do:

validate: new SimpleSchema({ currentName: { type: String }, advert: { name: { type: String, optional: true }, url: { type: String, optional: true, regEx: SimpleSchema.RegEx.Url }, imageUrl: { type: String, optional: true, regEx: SimpleSchema.RegEx.Url }, extraData: { type: String, optional: true }, } }).validator(),

— Reply to this email directly or view it on GitHub https://github.com/meteor/validated-method/issues/16#issuecomment-162968761 .

[stubailo]

OK I see where the boilerplate is here. I think this package was intended to be used like this:

AdManager = {};

AdManager.methods = {};

AdManager.methods.addAdvert = new ValidatedMethod({
  name: 'AdManager.methods.addAdvert',

  validate: new SimpleSchema({
    name: { type: String },
    url: { type: String, regEx: SimpleSchema.RegEx.Url },
    imageUrl: { type: String, optional: true, regEx: SimpleSchema.RegEx.Url },
    extraData: { type: String, optional: true },
  }).validator(),

  // Here we can reduce boilerplate by just having the whole argument, since we're just
  // inserting the whole thing into the DB anyway
  run(advert) {
    checkIsAdmin(this.userId);

    if (AdManagerAdverts.findOne({name: name}))
      throw new Meteor.Error('already-exists', 'An advert with this name already exists');      

    return AdManagerAdverts.insert(advert);
  }
});

I'm curious - why is Manager a class? It doesn't actually seem to do anything when it's instantiated? If Manager had a constructor and some internal state, then this code would look less like boilerplate.

I think the thing that makes this file look repetitive are:

1. Duplication between the class methods and Meteor Methods
2. Duplication between Method name property and the name of the actual Method object.

I think (1) can be removed by doing the above, (2) I'm not really sure how to get rid of.

[elie222]

Yeah. The class bit wasn't my issue so much. At the end of the code I write:

AdManager = new Manager();

And export AdManager. This is package code.

For anyone using the package, I didn't want them to have to write AdManager.methods.addAdvert() each time. AdManager.addAdvert() is easier. I just went with a class instead of a singleton object in case there was ever a need for multiple instances of manager in the future, but maybe your code is cleaner.

The only issue I had with this package were blocks such as these repeating themselves:

validate: new SimpleSchema({
    name: { type: String },
    url: { type: String, regEx: SimpleSchema.RegEx.Url },
    imageUrl: { type: String, optional: true, regEx: SimpleSchema.RegEx.Url },
    extraData: { type: String, optional: true },
  }).validator(),

And then you would also write similar code in the collection schema. Maybe that's just how it has to be though.

[stubailo]

Well if your collection schema is identical to your method schema, you can just use it:

validate: AdManagerAdverts.schema.validator()

So I don't think duplication between the collection and method schema is a problem? Or is there something else?

[elie222]

You're right! That's really simple. I didn't actually think about that. Would have to work something out for update methods. Also, validation would still run twice: once before the method, and once after it before insert.

[stubailo]

Also, validation would still run twice: once before the method, and once after it before insert.

Yeah, I think running the validation multiple times is a cost you kind of have to pay when you're guarding against programmer error in a dynamic language (if Methods and Collections could be statically typed, that would be pretty amazing!!). If this becomes a performance bottleneck in your app then we can solve it, but I bet it wouldn't be.

[elie222]

Yeah. Makes sense. Thanks for the pointers!

On 8 December 2015 at 21:30, Sashko Stubailo notifications@github.com wrote:

Also, validation would still run twice: once before the method, and once after it before insert.

Yeah, I think running the validation multiple times is a cost you kind of have to pay when you're guarding against programmer error in a dynamic language (if Methods and Collections could be statically typed, that would be pretty amazing!!). If this becomes a performance bottleneck in your app then we can solve it, but I bet it wouldn't be.

— Reply to this email directly or view it on GitHub https://github.com/meteor/validated-method/issues/16#issuecomment-162990156 .

[elie222]

I uploaded the package to Atmosphere. The repo is here: https://github.com/elie222/meteor-ad-manager

The update method is one ugly method: https://github.com/elie222/meteor-ad-manager/blob/master/lib/AdManager.js#L44 Also, it doesn't make too much sense to use the same schema a lot of the time for pre-method validation and post-method validation (post-method being right before the document is inserted/updated into the database. One case I noticed this popping up is optional fields with defaultValues. I should probably open this up as a new issue, but what was happening was a field such as this:

  disabled: {
    type: Boolean,
    defaultValue: false,
   // removing this line causes the validation to validated-method validation to fail
    optional: true
  }

(from: https://github.com/elie222/meteor-ad-manager/blob/master/lib/AdManagerAdverts.js#L32)

If I set optional to false then the initial validation fails.

In general you shouldn't have to do too much validation when the method is called. Just using check is good enough in my opinion. Simple schema is useful for when doc is actually inserted.

[stubailo]

Yeah I mean think of the method schema as an alternative to check. It's only a couple more characters to type.

The main idea here was that you shouldn't have to worry about using check and simple-schema for different parts of your code - you can just use one syntax to validate everywhere.