Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Feature request: allow server hooks on document insert, update, remove #395

Open
guillermosan opened this Issue · 57 comments
@guillermosan

Case use:

When a user in my app sends a new message i want to include in the doc the creation date of the item. I don't want to trust the client clock, it needs to be a server side generated value.

I've been doing something like:

myCollection.allow({
    insert: function (userId, doc) { 
        if (userId === doc.owner_id) {
            doc.creationDate = +(new Date);
            return true; 
        }
});

That works. Mongo insert includes the creationDate field and it's serverside generated.

Then i tried this:

Meteor.users.allow({
    insert: function (userId, doc) { 
            doc.someDefaultValue = true;
            return true; 
        }
});

This one fails. The insert function doesn't get called when users are inserted. I went at irc for some help and someone warned me this way of modifying the doc might not be intended, and that it is problably failing due to some default Meteor.users.allow call.

What would be the ad-hoc approach? For me this method works nicely.

@glasser
Owner

The fact that mutating arguments to allow functions works should not be relied on. And you've already discovered that Meteor only calls as many allow/deny functions as are minimally necessary to discover if a write is allowed: if one allow returns true (and no deny does), it won't bother to call others.

This does suggest that we should be able to add hooks to the default write methods (especially insert) to allow for this sort of customization.

Note though that for Meteor.users specifically:

  • There is a hook you can use to customize this: Accounts.onCreateUser.
  • Also, we actually automatically include a createdAt field on new users with the timestamp.
@glasser glasser closed this
@avital avital reopened this
@avital
Owner

Renamed this to be a feature request.

@avital
Owner
@pfafman

+1 on this. I have some similar code for insert and update but tied it to a 'deny' that returns false. Since, from my understanding, all of the deny's have to run and return false for the insert or update to work. But I agree this is a hack and would be nice to have some 'official' way of doing this.

@cazgp

+1. I need to delete some files from S3 when a document is deleted, and it would be nice to have an "onDelete" hook.

@awwx

@cazcazn:

collection.find().observe({
  removed: function (oldDoc) {
    // delete oldDoc._id from S3
  }
});
@matb33

+1. I wrote something in the meantime but it has higher chances of breaking in future updates to the API (monkey-patching _validatedInsert etc): https://github.com/matb33/meteor-collection-hooks. It also isn't elegant in its function signatures because it matches those of update, insert and remove, so you end up having to look at modifiers and re-querying against selector...

Ideally the official implementation would have the same hooks (before/after insert/update/remove), but the function signatures would look like allow/deny and observe.

That being said, my monkey-patch can be useful, as it allows for interesting hooks such as "before delete".

EDIT

I realized I could use observeChanges on both client and server, so the need for my hack isn't needed unless you're looking for a "before delete". That could perhaps just be done within deny then.

@matb33

@avital Are there any plans for an official implemention?

My current solution looks like this:

var test = new Meteor.Collection("test");

test.after("update", function (userId, selector, modifier, options, previous) {
    doSomething();
});

The function signature doesn't change -- you need to run a .find against selector to get the document(s) in question. I think an official implementation would fire on a per-document basis, just like allow/deny.

There's an after and before for each of the update, insert and remove (and recently find and findOne). This functionality is picking up steam as I'm finding more and more users are depending on it.

I've been writing some pretty large apps, and they have come to rely on this functionality quite heavily. I'm not just a casual Meteor user either -- these hooks have become essential to my Meteor development process in the professional realm.

Would you consider merging a solution to this into core if we were to produce an adequate one?

@mizzao

Also adding link to thread I started about this in meteor-core: https://groups.google.com/forum/#!topic/meteor-core/JGA_LQDEqw0

@raix

:+1: especially server-side hooks, eg. When want to compare the new and existing document eg. Not letting old charges overwrite latest. At the moment I'm overwriting the collection method calls on the server, but some official way could be Nice, https://github.com/raix/Meteor-GroundDB/blob/master/groundDB.server.js#L65

@mizzao

@raix That is the same approach taken in collection-hooks as well. We should definitely figure out a core-integrated API that will let us do all of these operations and without monkey-patching all the existing functions :)

@raix

Yep, cant rely on meteor naming methods /collection/insert - the docs speak of other ideas for naming - so I guess we would have to rewrite code at some point - bit of time wast

@cmather

Oops didn't realize @mizzao. On the plus side, I'm now probably more capable of helping your project. For what it's worth on this thread (my project is private so people won't see it there), my use cases are as follows:

  1. Add fields to a doc on client and server without sacrificing latency comp. this means I want an immediate insert into local cache. So using methods is out unless I duplicate most of the Collection RPC methods. Examples include inserting timestamps and caching user fields (e.g. on a comment). EDIT: Biggest use case on evmind right now is adding the text_body attribute of a comment to the comment list immediately, but on the server, passing the text_body to github for markdown processing which sets the html_body attribute. When it's ready, publish to the client.

  2. Kick off an email or a job on insert, update delete, on the server. This can also be accomplished with an observer as @awwx points out.

  3. Don't fire callbacks for ddp added/changed/removed messages. This means, I don't want the before/after hooks to be called on the client just because the LocalCollection was updated from a DDP message from the server. For example, there is no reason to fire the callbacks on each initial insert from all the records off the wire.

@mizzao

@cmather, this is actually @matb33's project. However, I need this quite a bit for my application and pushed a bunch of additional features in during the last week or two. I needed hooks that were different on a per-user basis and hence needed the userId in not just mutator methods but find/findOne as well.

The server-userId branch of https://github.com/matb33/meteor-collection-hooks is where we've been pushing some new stuff. As you can see, you were thinking along very similar lines with hooks on both server and client side (which could potentially be configured differently.) @matb33 has also taken the care to properly take care of the validated server-side hooks for client changes that don't use methods.

One other thing that we thought about doing, and might be a little more realistic now given the additional firepower provided by you, is to push a more comprehensive set of code to meteor that operates at the document level like allow/deny hooks and will be more future-proof without the monkey-patching.

At any rate, if you check out the current project, it's more than capable of doing all that you just described.

@raix

@cmather My use case is adding a server timestamp on client via time diff, even when offline - then on the server hook up to check that latest server timestamps dont get overwriten by older ones or have a history collection to add all changes for kind of a ot merge.

@matb33

I've rewritten collection-hooks from the ground up, with a focus on using Meteor core patterns to make it easier to merge later. By that I mean I copied then adjusted snippets of code from core instead of writing them with my own unique approach. The hope is that it will be easier for devs familiar with core to grok what I've done.

It's still a work in progress, and sitting on a branch: https://github.com/matb33/meteor-collection-hooks/tree/aop

It matches the same function signatures as allow/deny, which was the goal of this rewrite.

@mizzao

@matb33 that sounds great!

I checked out your code - I'm not joking when I say that I really appreciate the change from hard tabs to 2-space soft tabs. It was kind of frustrating to contribute before because I'd have to get my editor to replace all spaces with tabs. The new style you've adopted is much easier to work with and consistent with the Meteor core code style as well.

@raix

@mizzao btw. I made two files to add in projects for using meteor conventions (editor config and jshint conf) https://github.com/raix/Meteor-jshintrc most editors Got plugins for using these files - makes it easier to work together.

@mizzao

Now that Meteor is approaching v1.0 and collection-hooks has been stable for a while, I thought we might consider revisiting the idea of merging it into core.

As opposed to most packages which shouldn't be part of core, collection-hooks is a low-level API that actually enables a lot of very interesting things to be done to collections, including auto-timestamping, validation of data, etc. It's also something that is not naturally implemented outside of core and required some very weird monkey patching to get into Meteor. Right now both collection-hooks and things like Collection2 are monkey-patched, but I think most of the packages that modify the behavior of collections could use properly designed hooks to accomplish their objectives.

Thoughts? Ping @tmeasday as I know he's been using this for various purposes as well.

@tmeasday
Collaborator
@glasser
Owner

We do not use GitHub to track feature requests (other than for Blaze). The core team's current roadmap is at https://roadmap.meteor.com/. Discussions about features that users desire are great topics for the meteor-talk mailing list, where the community can help come up with solutions that don't require core changes.

We don't have any current plans to evaluate collection-hooks to bring it into core before 1.0. It's great to hear how much success Meteor users have had using it.

@glasser glasser closed this
@mizzao

@glasser I really don't agree with

Discussions about features that users desire are great topics for the meteor-talk mailing list, where the community can help come up with solutions that don't require core changes.

given how much monkey patching had to go into collection hooks for it to work. It's just not possible for it to be implemented in a clean way without core changes. Moreover, there are so many important functions that developers use that all are based on the functionality of collection hooks or by monkey-patching their own hooks.

Anyway, I'm tired of fighting for useful features through the mess that is the mailing list. It's not just me - there are lots of people with valuable ideas who are just throwing up their arms in frustration and having their thoughts go into the ether. I can't see this being good for the community, but it is what it is.

@awatson1978

Indeed. It's obvious that there's some frustration happening in various places, and some need for some new systems to be put in place. Some ideas:

Meteor is growing, and is having scaling pains right now. But there are many amazing community resources available. Lets find ways of reinforcing those community efforts. The developers at Pintask, Pushpin, Assistant, Telescope, and Vonvo are looking for adoption and feedback. The Meteor community is seeking tools to help it grow and scale. It's pretty obvious what needs to be done. ;)

@raix

I Think whats needed is a mdg member who can coordinate community and collect information about what should be changed in core, to make the community able to write packages - whitout monkey patching, overwriting etc.
Also make sure to coordinate efforts in the. Community so that we dont all write the same code / packages...

I dont Think yet another tool Will fix this, keep it simple.
I Think the coordinator Will help utilize the free ressources in the community, help us all save time and make meteor better.

Kind regards Morten

@avital
Owner

Thanks @raix, I agree. The solution is a human one, not a technical one.

We have started to do this with Blaze and plan to expand that to other parts of Meteor. At the moment though our core dev bandwidth is too constrained. This will likely change once packaging is released.

@mizzao

Would it be possible to re-open this issue (now that FRs are permitted) and consider the integration of https://github.com/matb33/meteor-collection-hooks into core? It's very widely used, yet requires a level of monkey patching and version dependence way beyond normal packages.

Moreover, the design and lessons we've learned from collection hooks will go a long way toward creating a solid and stable core API.

A set of proper hooks would allow for a much cleaner implementation of the https://atmospherejs.com/aldeed/collection2 package.

@glasser
Owner

Sure, reopened based on our new feature request guidelines.

@glasser glasser reopened this
@theduke

+1 for this, I see it as quite essential functionality.

@rclai

I am all for a non-monkey-patching solution to extending the Mongo.Collection API.

@almogdesign

+1 this would be great, would also solve some issues for me in offline mode with groundDB

@jasonxeno

+1 I use meteor-collection-hooks extensively in my pretty large app, it would be wonderful to unify it under Meteor core to increase stability. It's one of the most popular packages currently sitting at 3k on Atmosphere.

@rhyslbw

+1 The number of end users interacting with this code would be huge

@nrafter
@rcsandell

+1 has been very useful.

I've been using collection before hooks to modify all (server) finds used in publications to give the intersection of the users documents, and then whatever other selector I used in a find for a publication.

@stocksp

+1

@teknologist

+1 indeed. Extensively using collections hooks package here too.

@webyak

+1

@JackAdams

+1 for a non-monkey-patching solution to extending the Mongo.Collection API. Several super-useful and/or highly popular packages use this, but it's sometimes difficult to get them all working together due to the layers of wrapping of Mongo.Collection (with different functionality and APIs at each level of wrapping).

@risul

+1

@wursttheke wursttheke referenced this issue in matb33/meteor-collection-hooks
Closed

insert in .after.insert runs twice #104

@mizzao

The number of issues getting reported for collection-hooks is reaching epic levels: https://github.com/matb33/meteor-collection-hooks/issues

Most are not about code quality, but use, design choices, and edge cases. I think this makes a strong argument for integrating into core, given how prominently it is used.

@rclai

Yes, also, I just wanted to link this issue as well since it is related. Also, since Mongol was released, people have also been experiencing similar issues as well.

@Siyfion

I'd love to see an official hooks implementation added to the API.

@dweldon

:+1: Yes please.

@dandv

Server-side auto-timestamping and auto-adding the user who inserted a document are such common use cases that they're the first example in the collection2 autovalue documentation. Would love to see these made easy by the core.

@DeBraid

+1. Should have after insert hook for setting Session variables.

@gbachik

+1

@jms301

Yup auto-userId is my use case for this. The client should not need to know it's userId before submitting inserts.

@mcbain

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.