Skip to content
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

Allow new Mongo.Collection to be called multiple times for the same collection name and turn off defining mutation methods #5778

Merged
merged 1 commit into from May 17, 2016

Conversation

@aldeed
Copy link
Contributor

@aldeed aldeed commented Dec 11, 2015

  • No error is thrown when constructing a Mongo.Collection twice for the same name if you pass a new undocumented option to the constructor: _suppressSameNameError: true. When this option is used, if mutation methods were already created for a named collection, we skip creating them and do not throw an error.
  • Mongo.Collection constructor takes a new option, defineMutationMethods, which can be set to false to skip creating the default mutation methods
  • _defineMutationMethods function performance is slightly improved by returning earlier if there is no need to loop through method types
  • Previously constructing a second Mongo.Collection on the server with the same name would throw an error that didn't really explain what the true problem was. Now, instead of the confusing error saying "A method named /<name>/insert' is already defined, it says There is already a collection named "<name>", which is the same error that client code throws.

The reason for defineMutationMethods: false option is so that packages can subclass Mongo.Collection and provide their own validated mutation methods, turning off the default ones.

@projectn00b
Copy link

@projectn00b projectn00b commented Dec 11, 2015

Can we have collection hooks baked in?
On Dec 10, 2015 8:44 PM, "Eric Dobbertin" notifications@github.com wrote:

  • No error is thrown when constructing a Mongo.Collection twice for
    the same name
  • If mutation methods were already created for a named collection, we
    skip them and do not throw an error
  • Mongo.Collection constructor takes a new option,
    defineMutationMethods, which can be set to false to skip creating the
    default mutation methods
  • _defineMutationMethods function performance is slightly improved by
    returning earlier if there is no need to loop through method types

This could obviously have some edge cases effects I'm sure, but I can't
think of any and all tests still pass. Maybe something weird would happen
if defining it twice with different types of _id?

There is of course the chance that two packages, or a package and app,
unknowingly use the same collection name and don't get an error. That
possibility is not reason enough to keep throwing the error, though, IMO.
(In fact this error often bites me when I want to declare the same
mongo collection as a package because the package does not export their

reference.) Perhaps just a console.warn?

You can view, comment on, or merge this pull request online at:

#5778
Commit Summary

  • Allow new Mongo.Collection to be called multiple times for the same
    collection name

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#5778.

if (!ok)
throw new Error("There is already a collection named '" + name + "'");

// If ok is false, that means we already registered the store for this collection name, but

This comment has been minimized.

@serkandurusoy

serkandurusoy Dec 11, 2015

I think the best option would be a warnIfCollectionExists property on the Collection constructor and then a warning on the console if it set.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Dec 11, 2015

Can we have collection hooks baked in?

An easy way to write collection hooks in your app would be to use ES2015 classes to override certain methods on the collection, like: http://guide.meteor.com/collections.html#hooks

going to take a look at this PR today!

@serkandurusoy
Copy link

@serkandurusoy serkandurusoy commented Dec 11, 2015

override certain methods on the collection

@stubailo that would not be chained, though, right? The last override wins!

@stubailo
Copy link
Contributor

@stubailo stubailo commented Dec 11, 2015

@serkandurusoy yes - and there are big benefits to avoiding action at a distance like that. It could be pretty hard to understand what is going on in your app if any package or part of your app could add hooks to any collection.

@serkandurusoy
Copy link

@serkandurusoy serkandurusoy commented Dec 11, 2015

I use hooks heavily and often find the need to contextually divide them based on what kind of task the hook is going to do. I also heavilty use a pattern where client/server hooks run different tasks.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Dec 11, 2015

What do you use them for? Perhaps hooks are a crucial feature, it would be good to know what you're running into.

@serkandurusoy
Copy link

@serkandurusoy serkandurusoy commented Dec 11, 2015

A few frequent usecases I have for hooks (matb33:collection-hooks) are

  • Modify the document
    • Add default values
    • Apply constraints
    • Clean up some fields
    • Set or unset conditional fields (fields that depend on the value of another field or document or collection)
  • Creating notifications and/or messages, in the form of
    • email,
    • a record on a notifications collection,
    • a call to an api (internal or external)
    • analytics events
    • setting some reactive datasource on the client
  • Update some related item on another collection
    • some counts,
    • denormalized fields
    • continue on a series of actions that may or may not need to be within a transaction

These all do have other means of achieving the desired results, but I've grown quite fond of a hooks-based design because it does provide context around events and actions and allow me to centralize them, similar in some ways to the dispatcher pattern of flux.

PS: Please let me know if you want to carry this over to another thread.

EDIT:
I often end up using a combination of a number of the above and create different hooks for different tasks.

  • Sometimes they need to be different (before/after cases),
  • sometimes one needs to be explicitly on the client or the server due to its context,
  • and sometimes just due to the contextual difference of the task. Like, sending an email being very different than an analytics event or a database cleanup

@aldeed
Copy link
Contributor Author

@aldeed aldeed commented Dec 15, 2015

I don't know why CI tests failed but they all pass locally.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Dec 15, 2015

I just realized I haven't thought of this - what happens if you declare the same collection twice on the client? Do they both get the data?

@serkandurusoy
Copy link

@serkandurusoy serkandurusoy commented Dec 15, 2015

Hmm, right. Perhaps a better api would be for collection2 to provide something like baseCollection that just returns the non-decorated version of the collection.

@projectn00b
Copy link

@projectn00b projectn00b commented Dec 15, 2015

One of the major issues with matb33:collection-hooks is that you cannot do
a mongodb call within a hook. Say for instance you had to get a value from
mongodb to insert into before insert it would not allow it.
On Dec 11, 2015 1:47 PM, "Sashko Stubailo" notifications@github.com wrote:

What do you use them for? Perhaps hooks are a crucial feature, it would be
good to know what you're running into.


Reply to this email directly or view it on GitHub
#5778 (comment).

@serkandurusoy
Copy link

@serkandurusoy serkandurusoy commented Dec 15, 2015

@projectn00b how so? I use matb33:collection-hooks in usecases where I lookup extra information from other collections.

@aldeed
Copy link
Contributor Author

@aldeed aldeed commented Dec 15, 2015

@stubailo I think they both get the same data by pointing to the same store. Because registerStore will be called every time but exits pretty much immediately after the first time.

It's a good sign that existing tests pass, but maybe we need to think of some additional tests to add, checking that pub/sub works properly for multiple instances of the same collection. allow/deny should work fine since all client instances will call the same mutation method, but probably should have tests to prove that, too.

@aldeed
Copy link
Contributor Author

@aldeed aldeed commented Dec 15, 2015

Regarding hooks, my opinion is that they are useful in enough valid cases to be warranted. Making sure they aren't misused is an argument for good documentation of them, not entirely omitting them.

However, this PR, and in general supporting subclassing and custom mutation methods, opens the door to future packages that can provide hooks without it needing to be part of the core pkg.

@projectn00b
Copy link

@projectn00b projectn00b commented Dec 15, 2015

@serkandurusoy a simple find or findone usually yields an error when using
within the hook itself.
On Dec 15, 2015 4:19 PM, "Serkan Durusoy" notifications@github.com wrote:

@projectn00b https://github.com/projectn00b how so? I use
matb33:collection-hooks in usecases where I lookup extra information from
other collections.


Reply to this email directly or view it on GitHub
#5778 (comment).

@serkandurusoy
Copy link

@serkandurusoy serkandurusoy commented Dec 15, 2015

@projectn00b I assure you they work. If you would like to share your usecase, I'd like to offer you help. But let's do that either on SO or the matb33 issue tracker.

@ghost
Copy link

@ghost ghost commented Jan 24, 2016

I cannot understand why the test failed. circleci is not giving useful information so far..

I came to this topic because of another problem: When you try to create a new AccountsClient with a separate DDP connection, the AccountsCommon constructor tries to create a new Collection with the name "users".. see #6070

@zol
Copy link
Contributor

@zol zol commented May 10, 2016

Hey @aldeed . I'm happy to help get this merged if you still require the functionality but as per the comment history it looks like at least more investigation needs to happen regarding the behavior it introduces on the client (ideally, more tests). I'm keen to get this PR either closed or merged.

We've recently fixed up the CI should if you want to pick up working on it again, please rebase the PR against the latest devel branch.

@aldeed
Copy link
Contributor Author

@aldeed aldeed commented May 16, 2016

@zol I rebased and rewrote this as a completely backward compatible opt-in change since there are some questions about the effects.

  • New suppressSameNameError option can be set to true to see only a warning about collections with the same name already being defined
  • New defineMutationMethods option can be set to false to skip creating the allow/deny mutation methods.

@zol
Copy link
Contributor

@zol zol commented May 16, 2016

Hey, It looks like the tests are failing in our CI system. Do they pass locally?

Just one change (apart from tests passing) I'd like before merging this is to rename suppressSameNameError to _ suppressSameNameError and remove it from the docs. We can surface this option later down the track if it proves to be reliable.

…ollection name

- No error is thrown when constructing a Mongo.Collection twice for the same name if _suppressSameNameError option is true
- If mutation methods were already created for a named collection, we skip them and do not throw an error
- Mongo.Collection constructor takes a new option, defineMutationMethods, which can be set to false to skip creating the default mutation methods
- _defineMutationMethods function performance is slightly improved by returning earlier if there is no need to loop through method types
@aldeed
Copy link
Contributor Author

@aldeed aldeed commented May 17, 2016

@zol I changed to _suppressSameNameError. All tests pass locally but one test failed on Travis CI, "drop collection", which does not appear to be related to anything I did, but I'm not sure.

@abernix
Copy link
Member

@abernix abernix commented May 17, 2016

@aldeed That test is failing on a regular basis – it's probably (read: almost positively certainly) not your change. It worked fine on Circle but failed on Travis:

CircleCI:

S: tinytest - mongo-livedata - oplog - drop collection/db : OK

I just restarted your Travis test. Will see what happens.

@abernix
Copy link
Member

@abernix abernix commented May 17, 2016

🏁🎆

@zol zol merged commit 73b0564 into meteor:devel May 17, 2016
3 checks passed
3 checks passed
@apollo-cla
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zol
Copy link
Contributor

@zol zol commented May 17, 2016

Merged. Thanks guys!

zol pushed a commit that referenced this pull request May 17, 2016
@aldeed aldeed changed the title Allow new Mongo.Collection to be called multiple times for the same collection name Allow new Mongo.Collection to be called multiple times for the same collection name and turn off defining mutation methods May 17, 2016
@foobarbecue
Copy link

@foobarbecue foobarbecue commented Jun 17, 2016

Are there any contradindications / dangers associated with using _suppressSameNameError?

@aldeed
Copy link
Contributor Author

@aldeed aldeed commented Jun 18, 2016

@foobarbecue I have followed the code paths a bit and I don't personally see any problems with it, but the goal is to have people try it out and see if it causes any problems.

@foobarbecue
Copy link

@foobarbecue foobarbecue commented Jun 18, 2016

Cool, I've been using it on a project and haven't encountered any problems so far. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants