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

minimongo does not support observeChanges unordered with limit #1643

Closed
Tarang opened this Issue Nov 30, 2013 · 11 comments

Comments

Projects
None yet
4 participants
@Tarang
Contributor

Tarang commented Nov 30, 2013

I've found a couple of issues with server to server ddp. I'll make a sample project too that shows the issues but they're along the lines of this:

Server A

data = new Meteor.Collection("data");

data.allow({
    insert: function() { return true },
    update: function() { return true },
    remove: function() { return true }
});

Meteor.publish("data", function() {
    return data.find();
});

Server B

Server side js

var ddpconn = DDP.connect("http://otherserver");
ddpconn.subscribe("data");
var data = new Meteor.Collection("data", { connection: ddpconn });

Meteor.publish("data2", function() {
    var opts = {}

    // The first issue is with sorting uncomment line to see
    //var opts = {sort: {date: -1}, limit: 30}
    return data.find({}, opts);
});

Client side JS

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

Meteor.subscribe("data2");

//After subscribed test: doesn't work since there are no insert/remove/update methods
//on Server B

data.insert({date: new Date()});

The first error with publishing the collection with limit and sort gives back an error that there is no sort key provided.

The second error with inserts is that the methods (/collections/insert) can't be found on server b.

@glasser

This comment has been minimized.

Member

glasser commented Dec 6, 2013

This sounds serious but please provide a cloneable repo with both apps in it. For example, without knowing what packages are in your app (esp packages like insecure and autopublish) I can't know if I'm doing the same thing as you. I will reopen this bug if you post a complete replication with precise steps.

@glasser glasser closed this Dec 6, 2013

@Tarang

This comment has been minimized.

Contributor

Tarang commented Dec 7, 2013

Sure thing. I was going to just ran out of time that day

On 06 Dec 2013, at 23:54, David Glasser notifications@github.com wrote:

This sounds serious but please provide a cloneable repo with both apps in it. For example, without knowing what packages are in your app (esp packages like insecure and autopublish) I can't know if I'm doing the same thing as you. I will reopen this bug if you post a complete replication with precise steps.


Reply to this email directly or view it on GitHub.

@Tarang

This comment has been minimized.

Contributor

Tarang commented Dec 7, 2013

Here is a repo that will reproduce both issues:

https://github.com/Tarangp/meteor-ddp-bugs

  1. Run servera on port 3000, serverb on any other port (7000)

  2. To trigger the second I've made a button so just load the serverb app onto the browser with the js console open and press the only button on the page.

  3. To trigger the first bug uncomment the lines in /server/server.js in the serverb app and watch it's server console.

@glasser glasser reopened this Dec 8, 2013

@glasser

This comment has been minimized.

Member

glasser commented Dec 8, 2013

Bug 2 is "works as expected", I think. When you create a collection on the server with connection set, you're basically asking to act as a client for that connection rather than as a server. The docs for Meteor.Collection are a little muddled since they predate server-to-server DDP; they differentiate between "on the server" and "on the client" where they really should be talking about "on the server connecting directly to Mongo" vs "acting as a client to some DDP server".

Perhaps it would be nice to have a flag to Meteor.Connection on the server that means "make the mutation methods and proxy them through to the other server". It certainly shouldn't be the default, though!

As far as Bug 1 goes: yes, I'm not sure why that limitation exists. Maybe the unordered code doesn't support skip/limit? It dates from @n1mmy 's first implementation of minimongo observeChanges with skip/limit. That usage does seem perfectly reasonable to me. It's possible that we can just remove the

    if (!options._allow_unordered && !ordered && (self.skip || self.limit))
      throw new Error("must use ordered observe with skip or limit");

entirely? Renaming issue to match.

@testbird

This comment has been minimized.

testbird commented Dec 11, 2013

Regarding the "acting as a client to some DDP server" mode on server: Does this create, or allow to create, a minimongo live? DB on the server B (similar to clients), to enable for resyncs after a period of disconnection between server A and B?

The only ddp relay/proxy pointers I could find were open questions:
http://stackoverflow.com/questions/19594348/ddp-between-two-servers-doesnt-reconnect
http://stackoverflow.com/questions/18461456/forwarding-server-side-ddp-connection-collections-to-client

@testbird

This comment has been minimized.

testbird commented Dec 12, 2013

Hm, server B is "acting as a client to a DDP server". If B would actually hold real mongodb collections that it could sync back to server A (instead of just a minimongo cache), this would even be better!

-> persistent, indexed replica (relevant subset) closer to the client, disconected operation, two-way sync

What is your assessment, is this the use-case here, or possible?

@testbird

This comment has been minimized.

testbird commented Dec 12, 2013

I could think of these code paths making sense:

 new Collection:
   on server, connect to mongo database
      (specifying "{ connection: ddp_conn}" starts syncing up the (persistent) db)
   on client (browser), init minimongo
      (if no connection was specified, sync the (not yet persistable) database
        over the client's default ddp connection)
@glasser

This comment has been minimized.

Member

glasser commented Dec 31, 2013

There is a comment in _modifyAndNotify about an optimization that is made due to lack of support for skip/limit in unordered queries.

@glasser

This comment has been minimized.

Member

glasser commented Feb 27, 2014

Note that #1866 had a reproduction.

glasser added a commit that referenced this issue May 23, 2014

Only do one query for forEach/count in Deps
They used to run the query twice: once for the actual result and once to
set up the observe. Now it shares the work between the observe and the
actual query.

This required me to inline the _depend helper, but I actually think this
made what's going on more direct and clear.

Drop the _allow_unordered hack. I'm not convinced that it was ever truly
valid; the observe code really doesn't support unordered observes with
skip and limit, and I could not remember what it was about count's use
that made it hypothetically safe.  Easier to just remove the hack (until
we maybe eventually actually fix #1643)

Stop using Deps.Dependency in an unidiomatic way; just use
Deps.currentComputation directly.

@glasser glasser closed this in df2820f Jun 5, 2014

@glasser glasser reopened this Jun 5, 2014

@glasser

This comment has been minimized.

Member

glasser commented Jun 5, 2014

whoops, the auto-closer was a little over-zealous here!

@hwillson

This comment has been minimized.

Member

hwillson commented May 7, 2017

As discussed in this thread, Minimongo doesn't currently support using skip or limit with unordered observes. This is by design (not a bug). This limitation could be re-visited if there is enough interest, but there has been very little support for having ordered observe skip/limit functionality. As part of the on-going old issue cleanup process, I'll close this issue for now. Happy to re-open if anyone feels differently, but please keep in mind there has been no activity/interest in this issue for almost 3 years. Thanks for reporting this originally!

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