Add better handling for schema errors in Methods

[Bandit]

Took me ages to finally track this down. If collection2 denies an insert/update based on schema validation, nothing happens at all on the client, and nothing is printed in the server console.

This can be resolved by changing the best practice method code to listen for the error from collection2 as per its documentation

try {
  const documentId = doc._id;
  Documents.update(documentId, { $set: doc }, (error) => {
    if (error) throw new Meteor.Error('500', error.message);
  });
  return documentId; // Return _id so we can redirect to document after update.
} catch (exception) {
  throw new Meteor.Error('500', exception);
}

I think the boilerplate code should be updated accordingly?

[cleverbeagle]

Hi @Bandit. Not against this at all, however, I can see it tripping up things like inserts (using a callback would prevent getting back the new document _id). Have you tried that out yet or just updates?

[Bandit]

@cleverbeagle great point, I definitely didn't consider that. What's the best way to deal with schema validation then? As is we're better off not using collection2 if we can't get any visibility over its errors I would have thought?

What if we replaced the check with Collection.schema.validate(object) - that way we can catch any errors that schema validation throws up without affecting return values on our insert?

Documents.schema.validate(doc); // replaces Meteor's check(), and throws a Meteor.Error on failure

try {
  return Documents.insert({
    owner: Meteor.userId(),
    ...doc,
  });
} catch (exception) {
  throw new Meteor.Error('500', exception);
}

Thoughts? (As an aside it would help to keep the code DRY as repeating the object going to check() for every method is pretty tedious!)

[Bandit]

Looks like you also need to convert the default error from validate() into a Meteor.Error so that it gets returned to the client via Bert. As per the simple-schema docs:

SimpleSchema.defineValidationErrorTransform(error => {
  const ddpError = new Meteor.Error(error.message);
  ddpError.error = 'validation-error';
  ddpError.details = error.details;
  return ddpError;
});

[Edit] had to tweak it slightly to have it show the error message correctly in Bert

SimpleSchema.defineValidationErrorTransform((error) => {
  const ddpError = new Meteor.Error('validation-error', error.message);
  ddpError.error = 'validation-error';
  ddpError.details = error.details;
  return ddpError;
});

Not sure where the best place to put that would be? startup/server/schema-errors.js?

[pilarArr]

Hi @Bandit,
I also do the validation with the schema but I didn't do that transformation.
This is the code I use in the method, in case you find it useful.

try {
  Documents.schema.validate(doc); 
  return Documents.insert({ owner: Meteor.userId(), ...doc });
} catch (exception) {
  if (exception.error === 'validation-error') {
    throw new Meteor.Error(500, exception.message);
  }
  throw new Meteor.Error('500', exception);
}

I output the exception.message because I found it more useful than the exception.details in my case.

[Bandit]

@pilarArr nice, great idea.

How do you deal with AutoValues like DateCreated and DateModified? In my brief testing today they throw validation errors because the client doesn't run AutoValue functions? I'm considering throwing an optional: true on them but I'm 99% sure that's a hack!

[cleverbeagle]

@pilarArr @Bandit looking at this code, it seems like the separation is between the exception object having one of two properties: reason or message (like with the validation code above). Is that right? @pilarArr maybe we could come up with some sort of reusable function that checks which one of these is set and returns the appropriate value?

Edit: Documents.schema.validate(doc); this looks way cleaner than the check() stuff, which I'm into. Is just that alone enough to get the right error handling or do we still need to do the error type check?

[pilarArr]

@Bandit I had that problem too, I had to make a new schema to do the validation by picking the values I wanted to check, and then validate against that schema like this:

const newDocumentSchema = Documents.schema.pick('name', 'somekey1', 'somekey2');
newDocumentSchema.validate(doc);

I read the node-simple-schema docs, and you should be able to accomplish something like that using a named context on the schema to validate only certain keys depending on the validation context called but I never found out how to define named contexts for a schema by picking certain keys, so I just created new schemas for that. I have to create 2 different ones, one for updates and another for inserts because the updates needs the _id and the inserts don't.

@cleverbeagle I'll look into making that function, I've also run into an unnamed error in the client when doing inserts/updates that I don't see in either console that I have yet to trace, I'll keep you updated. I also likeDocuments.schema.validate(doc) but the linter does not like it when there is no check(), I'm not sure if Meteor does give an error when not using the check, you can see what I mean in their issue 88. Nevertheless I haven't seen that error yet. Also I'm not sure what you mean on the error type/error handling question.

[Bandit]

@cleverbeagle I think the best way to do it is as @pilarArr has done it in his example - by putting the schema.validate() in the try ... catch block. After that we just need as you say some smart error catching function to output a useful message to the client, as simple-schema puts this within error.message.

Putting schema.validate() outside the try ... catch is also valid, but you'll need to convert the error it throws into a Meteor.Error using the function I mentioned above, otherwise the client just reports a generic Internal Server Error.

The only obvious pitfall with using schema.validate() over check is that you have to put things like _id in your schema, and you run into difficulties on the client simulation for any keys with an AutoValue (because AutoValues aren't run on the client - not sure why they also aren't ignored by the client-side validate() but I digress). I'm yet to find the best solution for this, but there's no doubt in my mind people have encountered it before when manually validating a schema with Meteor.

[Bandit]

Have finally got a pattern working that I'm happy with. Keen to know your thoughts:

Items methods:

  'items.insert': function itemsInsert(doc) {
    const preparedDoc = {
      ...doc,
      owner: Meteor.user().selectedAccountId,
    };

    try {
      Items.schema.validate(preparedDoc);

      return Items.insert(preparedDoc);
    } catch (exception) {
      throwError(exception);
    }
  },
  'items.update': function itemsUpdate(doc) {
    const itemId = doc._id;
    const preparedDoc = {
      $set: _.omit(doc, ['owner']),
    };

    try {
      Items.schema.validate(preparedDoc, { modifier: true });

      Items.update(itemId, preparedDoc);

      return itemId; // Return _id so we can redirect to item after update.
    } catch (exception) {
      throwError(exception);
    }
  },

Items schema:

Items.schema = new SimpleSchema({
  _id: {
    type: String,
    label: 'The ID of this item',
    optional: true,
    custom() {
      // only required on an update
      if (!this.isSet && this.operator === '$set') return 'required';
    },
  },
  owner: {
    type: String,
    label: 'The ID of the account this item belongs to',
    optional: true,
    custom() {
      // only required on an insert
      if (!this.isSet && !this.operator) return 'required';
    },
  },
  createdAt: {
    type: String,
    label: 'The date this item was created',
    optional: true,
    autoValue() {
      if (this.isInsert) return (new Date()).toISOString();
    },
  },
  updatedAt: {
    type: String,
    label: 'The date this item was last updated',
    optional: true,
    autoValue() {
      if (this.isInsert || this.isUpdate) return (new Date()).toISOString();
    },
  },
  title: {
    type: String,
    label: 'The name of the item',
  },
  [...]
});

modules/throw-error.js:

import { Meteor } from 'meteor/meteor';

export default (exception) => {
  const message =
    (exception.sanitizedError && exception.sanitizedError.message)
    ? exception.sanitizedError.message
    : exception.message || exception.reason || exception;

  throw new Meteor.Error(500, message);
};

I had to remove the audit-argument-checks package to prevent Meteor from throwing an error due to the lack of check() within the new methods. Not sure about the security implications of this decision?

[pilarArr]

Hi @Bandit,

Your code looks good, a couple notes.

Either if you are validating via simple-schema or via check you have to define a schema for the item you are validating in both cases. The benefit of validating via simple-schema is that you can reuse the schema you have already created. I'm not sure that defining your own _id is a good idea, but I don't know about that.
You can't use the full schema you defined for your collection if you have autovalues defined because those are created when the item is inserted in the database, but you can pick elements from your schema to use in the validation, like this:

const newDocumentSchema = Documents.schema.pick('name', 'somekey1', 'somekey2');
newDocumentSchema.validate(doc);

And if you have to add an _id you can simply add this:

const editDocumentSchema = Documents.schema.pick('name', 'somekey1', 'somekey2');
editDocumentSchema.extend({ _id: String });

But if you solution works and you are happy with it, then that's it, I just like it better this way.

The problem with audit-argument-checks is addressed in this issue longshotlabs/simpl-schema/issues/88, to sum up, you should be able pass a flag to the validation method like this:

MyCollection.simpleSchema().validate(theObject, {check});

[cleverbeagle]

While I can see the importance of being able to reuse a schema, this seems like a lot of hoops to jump vs. writing the additional check() statements. With Pup the goal isn't to prescribe too much and this feels like it's going to be a lot to drop on folks.

That said, the errors problems seems manageable, perhaps just a matter of adjusting what value we throw for an error when something goes wrong. @pilarArr ignoring the check() statements and focusing only on the error/exception, is getting a useful message back for this something we can achieve with the current set up?

[OliverColeman]

So not being able to reuse the schema I'd already defined was driving me slightly crazy (DRY anyone?). Having to manually enter the field names to work-around the autoValues issue defeats the purpose to some extent. I ended up doing this:

/imports/api/Utility/server/schema.js:

const getSchemaFieldTypes = (schema, includeId) => {
  const typeMap = {};
  if (includeId) typeMap._id = String;
  
  Object.entries(schema).forEach(
    ([field, spec]) => {
      if (typeof spec == 'object') {
        if (!spec.hasOwnProperty('autoValue')) {
          typeMap[field] = spec.type;
        }
      }
      else {
        typeMap[field] = spec;
      }
    }
  );
  
  return typeMap;
}

export { getSchemaFieldTypes };

Then you can do something like this:

const DataSets = new Mongo.Collection('DataSets');
DataSets.schema = {
  name: {
    type: String,
    label: 'The name of the dataset.',
  },
  description: String,
};
DataSets.attachSchema(new SimpleSchema(DataSets.schema));

...
import { getSchemaFieldTypes } from '../Utility/server/schema';

Meteor.methods({
  'datasets.insert': function datasetsInsert(dataset) {
    check(dataset, getSchemaFieldTypes(DataSets.schema));
    
    try {
      return DataSets.insert(dataset);
    } catch (exception) {
      throw new Meteor.Error('500', exception);
    }
  },
  'datasets.update': function datasetsUpdate(dataset) {
    check(dataset, getSchemaFieldTypes(DataSets.schema, true));
    
    try {
      const datasetId = dataset._id;
      DataSets.update(datasetId, { $set: dataset });
      return datasetId; // Return _id so we can redirect to dataset after update.
    } catch (exception) {
      throw new Meteor.Error('500', exception);
    }
  },
  ...

[Bandit]

Clever 👍

Personally I'm fine with adding the optional flag onto my autoValue fields as a workaround, because as per the collection2 documentation, the server always overrides any client-side autoValue:

Note that autoValue functions are run on the client only for validation purposes, but the actual value saved will always be generated on the server, regardless of whether the insert/update is initiated from the client or from the server.

Your approach does have the added benefit of still utilising the check function so you don't have to remove the audit-argument-checks package though 😨

[OliverColeman]

It's certainly nice to be able to catch and display the errors like your approach does (as I've just found out ;)). Avoiding removing audit-argument-checks is also nice. Maybe something like this to combine both approaches:

/imports/api/Utility/server/schema.js:

import { Meteor } from 'meteor/meteor';

const getSchemaFieldTypes = (schema, doc, includeId) => {
  const typeMap = {};
  if (includeId) typeMap._id = String;
  
  Object.entries(schema).forEach(
    ([field, spec]) => {
      // Only consider top-level field schema specs.
      if (!field.includes(".")) {
        let optional = false;
        
        if (typeof spec == 'object') {
          if (!spec.hasOwnProperty('autoValue')) {
            typeMap[field] = spec.type;
          }
          
          if (spec.hasOwnProperty('optional')) {
        	optional = typeof spec.optional == 'function' ? spec.optional() : spec.optional || false;
          }
        }
        else {
          typeMap[field] = spec;
        }
        
        if (optional && !doc.hasOwnProperty(field) && spec.hasOwnProperty('defaultValue')) {
        	doc[field] = spec.defaultValue;
        }
      }
    }
  );
  
  return typeMap;
}

const throwSchemaException = (exception) => {
  const message =
    (exception.santizedError && exception.sanitizedError.message)
    ? exception.sanitizedError.message
    : exception.message || exception.reason || exception;

  throw new Meteor.Error(500, message);
};

export { getSchemaFieldTypes, throwSchemaException };

(I'm guessing getSchemaFieldTypes above will be missing various edge cases, however.)
Then:

Meteor.methods({
  'datasets.insert': function datasetsInsert(dataset) {
    check(dataset, getSchemaFieldTypes(DataSets.schema, dataset));
    
    try {
      DataSets.simpleSchema().validate(dataset);
      
      return DataSets.insert(dataset);
    } catch (exception) {
      throwSchemaException(exception);
    }
  },
  'datasets.update': function datasetsUpdate(dataset) {
    check(dataset, getSchemaFieldTypes(DataSets.schema, dataset, true));
    
    try {
      const datasetId = dataset._id;
      delete(dataset._id);
      
      DataSets.simpleSchema().validate(dataset);
   	  
      DataSets.update(datasetId, { $set: dataset });
      return datasetId; // Return _id so we can redirect to dataset after update.
    } catch (exception) {
      throwSchemaException(exception);
    }
  },
...

[cleverbeagle]

@OliverColeman this is really nice:

const throwSchemaException = (exception) => {
  const message =
    (exception.santizedError && exception.sanitizedError.message)
    ? exception.sanitizedError.message
    : exception.message || exception.reason || exception;

  throw new Meteor.Error(500, message);
};

It may be worth mentioning this in the simple-schema repo as I feel a lot of folks are frustrated by this, too.

[Bandit]

@cleverbeagle I wrote that - you can tell because there's a typo in it:

exception.santizedError && exception.sanitizedError.message => exception.sanitizedError && exception.sanitizedError.message

🤣

[cleverbeagle]

Logging this here: http://forum.cleverbeagle.com/t/schema-unique-document-fields/110/5?u=cleverbeagle.

Going to add this v.NEXT.