New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't alter data passed out of `observe` on the server side #209

Closed
tmeasday opened this Issue Jul 4, 2012 · 3 comments

Comments

Projects
None yet
2 participants
@tmeasday
Contributor

tmeasday commented Jul 4, 2012

Hey,

I've noticed that on the client side in minimongo you are very careful to always call LocalCollection._deepcopy on things as they come out of the Cursor (I suppose because the underlying objects are the data store). However, on the server-side you do not (at least for the results of observe).

This is a bug as I think users need to be able to alter the document that they receive. E.g. if I do this:

if (Meteor.is_server) {
  Users.find().observe({
    added: function(doc) {
      delete doc._id;
    }
  })
}

The following starts coming up in my logs (due to the LiveResultsSet having altered data):

Duplicate _id in old_results

It might seem obtuse to do that (I actually wanted to do it for my models), but I can certainly imagine situations where an observer might want just say add an attribute to the doc, in which case the next call to diff is going to think that an update happened and weird bugs might occur.

At the very least you'd need to tell users that they aren't allowed to alter documents.

@dgreensp

This comment has been minimized.

Contributor

dgreensp commented Aug 14, 2012

Can you rebase this against devel? Looks good otherwise :)

@ghost ghost assigned dgreensp Aug 15, 2012

tmeasday added a commit to tmeasday/meteor that referenced this issue Aug 15, 2012

My preferred solution to meteor#209
Conflicts:

	packages/mongo-livedata/mongo_driver.js
@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Aug 15, 2012

@dgreensp this is done in #276.

One thing you might want to note is that this change means _diffQuery is exclusively used with the 4th argument being true (i.e. always deepcopy).

I guess it's a generic utility function and someone might want to use it without copying at some point, but you might want to change the default at the very least (or simplify it and get rid of the argument).

@dgreensp

This comment has been minimized.

Contributor

dgreensp commented Aug 15, 2012

On devel, thanks Tom!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment