Meteor-Community-Packages / meteor-collection2

A Meteor package that extends Mongo.Collection to provide support for specifying a schema and then validating against that schema when inserting and updating.
https://packosphere.com/aldeed/collection2
MIT License
1.02k stars 109 forks source link

createdAt field #6

Closed mquandalle closed 11 years ago

mquandalle commented 11 years ago

Issue Initially posted in the autoform repository aldeed/meteor-autoform#12, I've realized that Collection2 is the right place for this use case.

I've defined a required createdAt field of type: Date in the schema of one Meteor.Collection2 which I'd like to populate just before inserting. To be trustable this field has to be added (or at least validated) on the server.

One solution is to create a myCollection2 that extends Meteor.Collection2, then overwrite the insert method to create the createdAt field before calling super. In coffeescript:

class myCollection2 extends Meteor.Collection2
    insert: (doc, args...) ->
        doc.createdAt = new Date 
        super(doc, args...)

Posts = new myCollection2 'posts',
    schema:
        title:
            type: String
        content:
            type: String
        createdAt:
            type: Date
            # Off topic, but I'd like to define a "immutable",
            # or "not update-able" constraint

if Meteor.isClient
    Posts.insert
        title: "Hello world"
        content: "This is a test"

One other solution might be to add some beforeInsert, beforeUpdate and beforeDelete hooks that take the doc/query and modify it before the real database operation is done.

Moreover I wonder if you would like to have some rails convention over configuration strategy. In other words, if a field called createdAt or updateAt of type Date is present in the schema, fill it automatically without any other code.

aldeed commented 11 years ago

I'm not sure I like the idea of that type of server callback in c2, but for this specific use case I think two features could be added.

  1. A value option in the schema, which can be set to a function that returns the value to be inserted for that key. It might actually have to be an object where you can set on: server or on: client, too, because certain values should come from client and others from server.
  2. Your suggestion of autofilling createdAt and updatedAt if present in schema. Additionally, it could autofill with now.getTime() if type is Number. And it should probably autofill only if not passed in explicitly.

Something to consider is whether these proposals are specific to collection2, or whether they should be implemented in the simple-schema package. collection2 pkg currently extends the simple-schema concept of a schema to support unique: true, which is clearly meaningless unless there is a collection involved. I'm pretty sure that these two proposals are similar and should be implemented in collection2, but I'm going to think about it a bit.

aldeed commented 11 years ago

Oh and regarding immutability, how about allowInsert and allowUpdate options that are true by default? These would be extensions to simple-schema implemented by the c2 package.

mquandalle commented 11 years ago

allowUpdate is a good option name. What is the use case for a allowInsert option?

mquandalle commented 11 years ago

Here is some examples I can think of using value and allowUpdate options in the schema:

{
    createdAt: {
        type: Date,
        value: function() {
            return new Date();
        },
        allowUpdate: false
    },

    updatedAt: {
        type: Date,
        value: function() {
            return new Date();
        }
    },

    checksum: {
        type: String,
        value: function(doc) {
            return Meteor._srp.SHA256(doc.content);
        }
    },

    updatesHistory: {
        type: [{date: Date, content: String}],
        value: function(doc) {
            var updatesHistory = doc.updatesHistory || [];
            updatesHistory.push({
                date: new Date,
                content: doc.content
            });
            return updatesHistory
        }
    },

    htmlContent: {
        type: String,
        value: function(doc) {
            if (Meteor.isServer) {
                return MarkdownToHTML(doc.MarkdownContent);
            }
        }
    }
}
aldeed commented 11 years ago

I guess potentially someone might want to define an updatedAt key that shouldn't be present unless an update has happened, so they could set allowInsert: false. I'm not sure if it's needed, just thought both should be supported.

Your examples look good. I'm assuming the doc passed to the value function would be the object that's being validated, either a doc or a mongo modifier object, right?

I'm OK with no autofill for now, too.

One remaining question:

How should client vs. server value functions be supported? Because in the case of returning a date, it would be weird to run in both places and then when the saved data is sent back from the server, it might have been the next minute on the server so a reactive display would show the time change. But I don't think it should be run only on the server because what if they want to use it to set some browser data or something from localStorage? Maybe just have developers check Meteor.isClient and/or Meteor.isServer within the function?

mquandalle commented 11 years ago

Maybe we can avoid to create allowInsert and allowUpdate options and just pass the operation type to the autoValue function as second parameter. If the function return nothing we just don't modify the document.

{
    createdAt: {
        type: Date,
        autoValue: function(doc, operation) {
            if (operation === 'insert')
                return new Date();
        }
    },

    updatedAt: {
        type: Date,
        autoValue: function(doc, operation) {
            if (operation === 'update')
                return new Date();
        }
    }
}
aldeed commented 11 years ago

I'm OK with either value or autoValue.

I was thinking allowInsert and allowUpdate might be used with other fields that are not autovalued as well, in which case passing into autoValue alone is not enough. But maybe the valueIsAllowed function could be used for that. Maybe we could set the this context to an object that indicates whether it's an update or insert, and do that for both autoValue and valueIsAllowed:

{
    createdAt: {
        type: Date,
        autoValue: function(doc) {
            if (Meteor.isServer && this.isInsert) {
                return new Date();
            }
        },
        valueIsAllowed: function (val, doc) {
            return this.isInsert;
        }
    },

    updatedAt: {
        type: Date,
        optional: true,
        autoValue: function(doc) {
            if (Meteor.isServer && this.isUpdate) {
                return new Date();
            }
        },
        valueIsAllowed: function (val, doc) {
            return this.isUpdate;
        }
    },

    immutableSecondaryID: {
        type: Number,
        valueIsAllowed: function (val, doc) {
            return this.isInsert;
        }
    },
}
aldeed commented 11 years ago

Hmm, or not. I just realized that valueIsAllowed is called by simple-schema, which doesn't have the concept of update vs. insert. True, it can guess fairly well based on whether there are $ operators present in the object, but I'd rather not get into messiness like that.

So I guess I'm still leaning toward allowInsert and allowUpdate, although we can also do this.isInsert and this.isUpdate in the autoValue function because that would be useful, too.

mquandalle commented 11 years ago

I think that allowInsert and allowUpdate options are simple to understand, not too difficult to implement and independent of the autoValue option (one could for instance deny the update of the field author of a message collection2).

I'm not sure if binding the autoValue function with this.isInsert and this.isUpdate is necessary. Do you have an example?

aldeed commented 11 years ago

It would be necessary because if you have allowUpdate: false, the autoValue function would still automatically set the value and then you'd get a validation error. Although I suppose the calling code could be written such that it skips the autoValue call if it sees that allowUpdate is false and it knows it's an update. So maybe it's not necessary.

aldeed commented 11 years ago

I think it will be a couple days before I'll have a chance to do this development. If you feel like doing it sooner, you can.

mquandalle commented 11 years ago

I will try. Do you have any advice on the implementation?

Yes I think if denyUpdate is set to true, we do not re-run the autoValue function.

aldeed commented 11 years ago

Autovalue stuff would happen right before the doc is cleaned in _insertOrUpdate. Have to check whether it's update and updates are not allowed or insert and inserts are not allowed, if neither is true then find all the schema keys that have autovalue set and run those functions, setting the return value into doc. For the "setting the return value into doc" piece for an update you'll have to set the value under $set, potentially creating the $set object if it isn't there. Might also have to check for other operators for that field and remove them, for example making sure that the user didn't also pass in $unset (or any other operator, really) for that field. Could get tricky.

For allow/deny, I'm not sure. I think the ideal place to do the check would be in the self._simpleSchema.validator function that's at the end of the constructor, but I'm not sure how to inform that function of whether it's an insert or update. Maybe set some flag on the c2 and then in validator you can get it from self through closure?