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

Add DDP._CurrentPublicationInvocation and DDP._CurrentMethodInvocation #8629

Merged
merged 17 commits into from
May 31, 2017
Merged

Add DDP._CurrentPublicationInvocation and DDP._CurrentMethodInvocation #8629

merged 17 commits into from
May 31, 2017

Conversation

zimme
Copy link
Contributor

@zimme zimme commented Apr 24, 2017

This pull-request is related to issue #8031. Please see this comment for more information.

Tests are still needed before we can merge this, but I thought I should try and get the ball rolling based on the clarification and discussion in #8031.

#8031 should probably be merged before this one if we decide to go down this road.

/cc @abernix, @mitar

@zimme
Copy link
Contributor Author

zimme commented Apr 24, 2017

Added a test to make sure that methods called from a publication have access to the connection.

randomSeed = DDPCommon.makeRpcSeed(currentInvocation, name);
} else if (currentPublicationInvocation) {
userId = currentPublicationInvocation.userId;
setUserId = function(userId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should be set in this case?

Copy link
Contributor Author

@zimme zimme Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we handle the case when a method wanna change the userId of the connection when the method was invoked by a publication?

Print the same kind of error like we do when setUserId is invoked in a method initiated from server-code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the potential downsides of changing the connection's userId in this case?
Will this result in security flaws or something else maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. You are doing the same as what initial method invocation is doing.

@mitar
Copy link
Contributor

mitar commented Apr 24, 2017

Can you make some tests which make sure DDP._CurrentMethodInvocation and DDP._CurrentPublicationInvocation are never set at the same time?

@@ -5,4 +5,5 @@ LivedataTest.SUPPORTED_DDP_VERSIONS = DDPCommon.SUPPORTED_DDP_VERSIONS;
// This is private but it's used in a few places. accounts-base uses
// it to get the current user. Meteor.setTimeout and friends clear
// it. We can probably find a better way to factor this.
DDP._CurrentInvocation = new Meteor.EnvironmentVariable;
DDP._CurrentMethodInvocation = DDP._CurrentInvocation = new Meteor.EnvironmentVariable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split this into two lines. One creating DDP._CurrentMethodInvocation and the other setting DDP._CurrentInvocation and commenting that this for backwards compatibility since X version because it was used as a public API by many packages.

We should maybe also change everywhere else in the core code to use DDP._CurrentMethodInvocation. But maybe somebody else from MDG could comment on this first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will fix that.

@zimme
Copy link
Contributor Author

zimme commented Apr 24, 2017

Can you make some tests which make sure DDP._CurrentMethodInvocation and DDP._CurrentPublicationInvocation are never set at the same time?

In the case when a method is called from a publication both DDP._CurrentMethodInvocation and DDP._CurrentPublicationInvocation is actually set. In this case DDP._CurrentMethodInvocation is created from DDP._CurrentPublicationInvocation.

And with the merge of #8031, there will be a test that DDP._CurrentInvocation/DDP._CurrentMethodInvocation will never be set in a publish function.

@mitar
Copy link
Contributor

mitar commented Apr 25, 2017

In the case when a method is called from a publication both DDP._CurrentMethodInvocation and DDP._CurrentPublicationInvocation is actually set. In this case DDP._CurrentMethodInvocation is created from DDP._CurrentPublicationInvocation.

Oh, you are right. Before it was the same, only that connection was null. And now we populate it from DDP._CurrentPublicationInvocation. Sounds good.

@mitar
Copy link
Contributor

mitar commented Apr 25, 2017

You should probably prepare a HISTORY.md entry as well. Because this could be a breaking change. How should we handle this? Probably it could be a part of next big release, 1.5?

@zimme
Copy link
Contributor Author

zimme commented Apr 25, 2017

I'll hold off on working more on this until we get an "official" stamp of approval.

@zimme
Copy link
Contributor Author

zimme commented May 3, 2017

You should probably prepare a HISTORY.md entry as well. Because this could be a breaking change. How should we handle this? Probably it could be a part of next big release, 1.5?

I don't really see this as a breaking change as DDP._CurrentInvocation is still there in it's current form.

@zimme
Copy link
Contributor Author

zimme commented May 3, 2017

Also I believe #8031 and this have competing tests. #8031 checks that this.connection is never set in a method called from a publication and this PR makes it available.

@mitar
Copy link
Contributor

mitar commented May 3, 2017

It is a breaking change because now connection is set in server method calls in some cases when it was not before.

@mitar
Copy link
Contributor

mitar commented May 3, 2017

Yes, we should probably change those tests to make sure that DDP._CurrentInvocation is not set inside publish, and do not really check what is inside a method. And then this tests here can check that inside methods connection is available both when calling it first time and after user was set.

if (!currentInvocation)
throw new Error("Meteor.userId can only be invoked in method calls. Use this.userId in publish functions.");
throw new Error("Meteor.userId can only be invoked in method calls or publications.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also change "This function only works if called inside a method." comment above. And maybe change the rest of the comment to warn that if you use Meteor.user() (not Meteor.userId()) outside of the reactive context on the server the data returned by Meteor.user() will not change reactivelly. I mean, to me this is pretty obvious, so maybe we should just remove this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try and generalize this comment a bit as this holds true for methods as well, as far as I can tell.
i.e. Meteor.user() won't be reactive on server-side either in a method or a publication.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think the argument here is that publications are longer active, so there is more chance that content returned by user changes.

BTW, I have a package which gives server-side reactivity inside publish functions, so then Meteor.user() is reactive. :-)

// _setUserId is normally called from a Meteor method with
// DDP._CurrentInvocation set. But DDP._CurrentInvocation is not expected
// to be set inside a publish function, so we temporary unset it.
// Otherwise, this can lead to a problem that if a publish function is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the part of the comment from here on, because it is not anymore true. connection will now be always set in server-side methods, but we do not want DDP._CurrentInvocation to be set for those who are accessing it directly to check if they are inside a method call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhere along the line I guess I merged #8031 and pushed to this PR which I didn't want to do. I'm thinking I'll rebase this branch of your PR and update the comments and tests to work with my changes. I don't really see these changes making sense without the changes in your PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you do not have to rebase, you merged and we can merge this one together. And we can just close that other one. I do not mind.

});

Tinytest.addAsync(
"livedata server - no connection in a method called from a publish function",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be probably renamed.

if (! methodCallResults[connectionId]) {
methodCallResults[connectionId] = [];
}
methodCallResults[connectionId].push(Meteor.call('livedata_server_test_inner'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here DDP._CurrentInvocation.get() shoud be pushed to methodCallResults now, no?

@zimme
Copy link
Contributor Author

zimme commented May 7, 2017

  • Support DDP._CurrentMethodInvocation, DDP._CurrentInvocation is kept for backwards-compatibilty.
  • Support DDP._CurrentPublicationInvocation, DDP._CurrentMethodInvocation is never set in a publication, even during re-run because a user logged in.
  • Method calls invoked from publications now inherit the connection and a few other things from the publication.
  • Meteor.userId() and Meteor.user() is now available in publications too.

There is one breaking change here.
That is that this.connection is set in methods that are called from a publication. Before it was not set, except in a rare case when a publication was re-run because a user logged in.
For apps that currently checks if this.connection is available in a method to determine if the method was called from "secure" server code this change will make the apps "more secure" because now these methods called from publications will not run as "secure" server methods as they now have this.connection.

@zimme
Copy link
Contributor Author

zimme commented May 7, 2017

I rebased this PR from #8031, so we should be able to just merge this PR and skip that one.

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimme Sorry for the delay in getting to this and thank you very much for visiting both this and #8031 and the additional insight/rational on both. This appears to be headed in the correct direction, and addresses my concerns with #8031 while still implementing the very important fix that @mitar provided there.

I'm not too concerned about the one breaking change given that the result of a pub => method will be a more-controlled permission set, and we can make this clear in History.md – though I do think we need a very clear call-to-action, for example:

If you Method.call from within a Meteor.publish and rely on the value of this.connection within those invoked methods... ... ...

@mitar I believe your (valid) comments above have been addressed after the recent round of commits, did you want to take another pass at it?

Excited to get this wrapped up!

// in a method or publish function is to do Meteor.find(this.userId).observe
// and recompute when the user record changes.
var currentInvocation = DDP._CurrentPublicationInvocation.get();
currentInvocation = currentInvocation || DDP._CurrentMethodInvocation.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe avoid the reassignment here and change currentInvocation to a const?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to reverse the order of the fallback here so that _CurrentMethodInvocation falls back to _CurrentPublicationInvocation (instead of publication falling back to method) since _CurrentMethodInvocation is (temporarily) unset within the publication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would make sense as it's more likely that DDP._CurrentMethodInvocation would be set. If that isn't set, we're in a publish function and we fall back to DDP._CurrentPublicationInvocation.

So given the first comment I would change this to const currentInvocation = DDP._CurrentMethodInvocation.get() || DDP._CurrentPublicationInvocation.get();.

setUserId = function(userId) {
currentInvocation.setUserId(userId);
currentPublicationInvocation._session._setUserId(userId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way to handle this? Or should this be more defensive? In other words, could currentPublicationInvocation._session ever be undefined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell a Subscription always have _session which is a Session and _setUserId on Session is what's used by a methodInvocation to set it's connections userId. We need this to be called properly so that publications are re-run when changing the userId.

I haven't found a different way of doing this, however maybe we could cc someone at MDG that knows more about the DDP protocol implementation to verify that _session is always available before merging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also looked into this because it was suspicious to me, but I could not find a cleaner way either. It seems the best and most stable one.

Maybe we should make a test which makes sure you call setUserId from inside a method called from publish function to make sure this will be always available. It would also in general be interesting to see what happens if you call setUserId from inside a publish function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what I can tell there is no setUserId on the publication context, that's why we need to reach into _session to get that function. Calling _session._setUserId from inside a publish function should just change the current connections userId and the publication should re-run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just make a test that setUserId can be called from a method called from a publish function. To catch if _session will be renamed at any point in the future or anything like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually already do this. Here.

Copy link
Contributor

@mitar mitar May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are great! I have no further comments.

@zimme
Copy link
Contributor Author

zimme commented May 8, 2017

I have found myself writing DDP._CurrentPublishInvocation during the development of this.

Maybe this is an indicator that we should switch from DDP._CurrentPublicationInvocation to DDP._CurrentPublishInvocation. I do, however, like the former better as a description.

History.md Outdated
re-run after a user logged in. This would cause method calls from within
publish functions to unexpectedly having `this.connection` available.
* Support `DDP._CurrentPublicationInvocation` and `DDP._CurrentMethodInvocation`
, `DDP._CurrentInvocation` is kept for backwards-compatibility. This change
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange wrapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just a line length thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but comma could be in the previous line, no?

@mitar
Copy link
Contributor

mitar commented May 8, 2017

I went through and it looks OK with my comments addressed, but I didn't do it as thoroughly as previously.

},

livedata_server_test_outer: function () {
return Meteor.call('livedata_server_test_inner');
},
livedata_server_test_setuserid: function (userId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a newline above that. Just a nitpick. ;-)

@zimme
Copy link
Contributor Author

zimme commented May 8, 2017

There, I believe all the current comments have been addressed.

I'm not sure if anything else is needed to get this merged, but I don't think I'll have time to work a lot on this until next week again.

@mitar
Copy link
Contributor

mitar commented May 9, 2017

I think we should add tests (and potential code improvements) which make sure that DDP._CurrentPublicationInvocation is set in all callbacks from publish functions, like onStop. I would even add a test to check (and make it so) so that it is set inside added, changed, removed callbacks from observeChanges callback from queries made inside publish functions.

In a way, you want connection to be available even in methods called from those callbacks.

Maybe that already happens because they are run inside a fiber, but it would still be good to test and assure that this is really so.

@zimme
Copy link
Contributor Author

zimme commented May 9, 2017

That's a valid point, I just recently fixed a bug in reywood:publish-composite #105
where DDP._CurrentInvocation wasn't available in the observe and observeChanges callbacks and I had to bindEnvironment.

However, I do believe this is another issue in itself. Becuase from what I can tell from working on that issue on this issue this is the way it currently work and to get these changes in quicker I would suggest we open a new issue for binding the environments of the callbacks.

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thank you, everyone, for all the hard work that went into this. This will come out in 1.5.1 due to the number of packages affected and its ties to the meteor package.

@abernix abernix merged commit 8005532 into meteor:devel May 31, 2017
@zimme zimme deleted the zimme/ddp-current-invocation branch May 31, 2017 15:29
mitar added a commit that referenced this pull request Jun 1, 2017
abernix added a commit that referenced this pull request Jun 1, 2017
Some formatting changes to (try! and) match existing `History.md` formatting.
abernix pushed a commit that referenced this pull request Jun 1, 2017
* Updating history a bit for #8629.

* Tweaks for #8629.

Some formatting changes to (try! and) match existing `History.md` formatting.

* Place related bullets next to each other.

For continuity in reading.
GulajavaMinistudio added a commit to GulajavaMinistudio/meteor that referenced this pull request Jun 1, 2017
@zimme
Copy link
Contributor Author

zimme commented Jun 7, 2017

I realized that we haven't bumped the versions of the packages this PR touches. Thinking that ddp-server might need a major version bump as this brings a potential breaking change for some developers.

@laosb
Copy link
Contributor

laosb commented Jun 10, 2017

Shall we update documents to use Meteor.user() / .userId() in publications?

@zimme
Copy link
Contributor Author

zimme commented Jun 10, 2017

Where in the docs are you talking about?
The @locus was updated in this PR which should change the docs as to where you can use Meteor.userId()/Meteor.user().

@laosb
Copy link
Contributor

laosb commented Jun 11, 2017

I mean, for example, http://docs.meteor.com/api/pubsub.html#Subscription-userId and some other places use this.userId in examples.

The real problem is, should we recommend Meteor.userId()/Meteor.user() in publish functions or just keep it as is while you can now use Meteor.userId()/Meteor.user() as an alternative?

@zimme
Copy link
Contributor Author

zimme commented Jun 11, 2017

@laosb, I'm thinking that we shouldn't recommend Meteor.userId()/Meteor.user() in publications as developers might expect those to be reactive but in reality they're just re-run if a user logs in or out.

I don't think it belongs in the Subscriptions section and looking through the Methods section there is no mention of Meteor.userId() or Meteor.user() that I can find.

But maybe we should make a note about all of this in the Accounts section which documents Meteor.userId()/Meteor.user()? i.e. you can use Meteor.userId()/Meteor.user() inside publications and methods, BUT think about this bla bla.

abernix added a commit that referenced this pull request Jun 12, 2017
* Major version bump for `ddp-server`
* Minor version bumps for:
  - `accounts-base`
  - `allow-deny`
  - `ddp-client`
  - `ddp-common`
  - `meteor`
  - `mongo`
abernix added a commit that referenced this pull request Jun 13, 2017
* Major version bump for `ddp-server`
* Minor version bumps for:
  - `accounts-base`
  - `allow-deny`
  - `ddp-client`
  - `ddp-common`
  - `meteor`
  - `mongo`
@mitar
Copy link
Contributor

mitar commented Jun 26, 2017

BTW, how does one now know if a method is called from an existing server-side method, or directly from a client side?

@zimme
Copy link
Contributor Author

zimme commented Jun 27, 2017

With the new code one would check if DDP._CurrentPublicationInvocation.get() returns null, if it does, it's invoked from the client otherwise it would have been invoked from a publication.

@zimme
Copy link
Contributor Author

zimme commented Jun 27, 2017

Ooh, I misread that quite a lot. How would you do it before? I guess that would still hold true? Also, what's the use-case?

@zimme
Copy link
Contributor Author

zimme commented Jun 27, 2017

One way would be adding an extra argument to the method, which by default is false and then you set it when you call the method from another method.

@mitar
Copy link
Contributor

mitar commented Jun 27, 2017

Before you checked that this.connection is true.

The use case is having a Meteor method which can have a more permissive API if called from the server side. For example, you can create a document without linking it to the current user (because you are calling it from the server side and you know that you will in the next step assign custom user as an author).

So I can have Meteor method createDocument(authorId) and when called from client-side this authorId has to match this.userId but if called from server side it can be any authorId.

Maybe the solution is to refactor this code and move most of the method into an importable function, and then inside Meteor method just check if authorId matches this.userId.

@zimme
Copy link
Contributor Author

zimme commented Jun 27, 2017

If a method is called from server-side code it won't have this.connection, however if it's called from another method which was invoked by a client, then it will have this.connection which should be seen as being invoked by the client directly, right?

I would go with the latter suggestion.

@mitar
Copy link
Contributor

mitar commented Jun 27, 2017

Yes, before calling a method from a method called from the client side behaved like a server-side call.

@abernix
Copy link
Contributor

abernix commented Jul 25, 2017

I guess we should probably open some PRs with the Docs and Guide to make it clear that it's no longer necessary to use this.userId? e.g. here, etc..

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

Successfully merging this pull request may close these issues.

6 participants