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

auto fetch for cached object #278

Closed
dirkmc opened this issue Aug 2, 2013 · 11 comments
Closed

auto fetch for cached object #278

dirkmc opened this issue Aug 2, 2013 · 11 comments

Comments

@dirkmc
Copy link

dirkmc commented Aug 2, 2013

I have a Post model with a related Channel, and the Channel model has related Users.
The Post, User and Channel models have autoFetch true and autoFetchLimit 1 (so that if I get a Post it will fetch Post.user but not Post.user.channels).

  1. If I get a Channel directly, the users are there.
  2. If I get a Post that has that Channel first, then get the Channel directly, the Channel does not have the users.

I'm guessing this is because in the second case the Channel object is put in the cache by Post.get() and then when it's retrieved, its related models aren't auto-fetched.

db.Channel.get(1, function(e,c) {
    console.log('Channel', c.name, 'with', _.size(c.users), 'users');
    // Channel MyChannel with 17 users
});

db.Post.get(1, function(e,p) {
    db.Channel.get(1, function(e,c) {
        console.log('Channel', c.name, 'with', _.size(c.users), 'users');
        // Channel MyChannel with 0 users
    });
});
@dresende
Copy link
Owner

Please post a simple version of your models and associations.

@dirkmc
Copy link
Author

dirkmc commented Aug 20, 2013

Here's a complete example:

define: function (db, models) {
    models.User    = db.define("users",    { email: String }, { autoFetch: true });
    models.Channel = db.define("channels", { name: String },  { autoFetch: true });

    models.User.hasMany("channels", models.Channel, {
        is_admin: Boolean,
    }, {
        reverse: "users", // Other side of many-to-many
        mergeTable: "subscriptions",
        mergeId: "user_id",
        mergeAssocId: "channel_id"
    });

    models.Post = db.define("posts", {
        context: String,
        user_id: Number,
        channel_id: Number
    }, {
        autoFetch: true
    });

    models.Post.hasOne("user", models.User);
    models.Post.hasOne("channel", models.Channel);

    // If you uncomment this it works, because the channel auto-fetches
    // its users (and puts them in the cache)
    /*
    models.Channel.get(1, function(e,c) {
        console.log('Channel', c.name, 'with', _.size(c.users), 'users');
        // Channel MyChannel with 17 users
    });*/

    // But this doesnt work, because the post auto-fetches the channels,
    // but not their associated users, so when the channel is fetched from
    // the cache, it has no users.
    models.Post.get(1, function(e,p) {
        models.Channel.get(1, function(e,c) {
            console.log(c.users); // undefined
            console.log('Channel', c.name, 'with', _.size(c.users), 'users');
            // Channel MyChannel with 0 users
        });
    });
}

@notheotherben
Copy link
Collaborator

Try setting autoFetchLimit: 2 on the Post model and see if that works.

@dirkmc
Copy link
Author

dirkmc commented Aug 21, 2013

I'm sure that will work, but it's not really a fix, it's a workaround :)

@notheotherben
Copy link
Collaborator

It's technically the way it should work. In order to avoid infinite recursive auto fetch operations, we limit the depth of items to auto fetch from the database. If you want to fetch items more than 1 level deep then you need to increase this depth limit.

If you are proposing that we auto fetch every item, regardless of the depth (and taking into account that each auto fetch operation is an additional DB query) imagine what would happen with the following situation.

var commit = db.define('commit', {
    hash: { type: 'text', required: true, size: 32 }
}, {

});

commit.hasOne('parent', commit, {
    autoFetch: true
});

Now imagine you have a database with 10000 commits in it, and you fetch a single commit (say, the 1000th one). ORM would have to perform an additional 9000 queries before you would get your commit back. Obviously this isn't ideal default behavior in any situation.

@dirkmc
Copy link
Author

dirkmc commented Aug 21, 2013

I understand what you're saying. I'm not suggesting that auto-fetch should be without limits.
My example above is like this:
Post -> Channel -> User
Post -> Channel is auto-fetch
Channel -> User is also auto-fetch
So what I'm suggesting is that when Post auto-fetches the Channel it should in turn trigger Channel to auto-fetch User.

@notheotherben
Copy link
Collaborator

And the solution is to set the auto fetch depth to 2, allowing ORM to auto-fetch the second level of elements (Channel -> User) when it fetches a Post element from the database.

@dirkmc
Copy link
Author

dirkmc commented Aug 21, 2013

I think maybe the problem isn't clear from the example I posted above. Consider this code:

onMessageReceived: function() {
    models.Post.get(1, function(e,p) {
        // Do something with the post and its channels,
        // don't care about the channel's users
    });
}

onTimerExpired: function() {
    // Now I just want to do things with a channel and its users
    models.Channel.get(1, function(e,c) {
        // But because of the caching issue, users are undefined here
        console.log(c.users); // undefined
        console.log('Channel', c.name, 'with', _.size(c.users), 'users');
        // Channel MyChannel with 0 users
    });
}

If onTimerExpired fires first and onMessageReceived fires second this code works fine
If onMessageReceived fires first and onTimerExpired fires second then channel.users will be undefined

@dresende
Copy link
Owner

Try changing onTimerExpired to avoid cache.

models.Channel.get(1, { cache: false }, function (e, c) {
    // ..
});

@dirkmc
Copy link
Author

dirkmc commented Aug 26, 2013

I feel like that's still not really getting to the root cause of the problem.

I've decided not to use auto-fetch directly on the models, only on specific queries, to avoid these kinds of problems.

@dresende
Copy link
Owner

I change how hasOne cache objects in #339, please try the latest commit.

@dxg dxg closed this as completed May 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants