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

Failing unit test about multiple instantiation (shouldn't fail) #180

Closed
philfreo opened this Issue Jul 31, 2012 · 10 comments

Comments

Projects
None yet
3 participants
@philfreo
Collaborator

philfreo commented Jul 31, 2012

This (new) test fails but shouldn't on the latest master.

If the Company employees relational did not have a reverseRelation then this problem goes away (but other tests fail).

test("Initializing multiple models and a collection should not break existing models", function () {

    var dataCompanyA = { 
        id: 'company-a',
        name: 'Big Corp.',
        employees: [ { id: 'job-a' }, { id: 'job-b' } ]
    };
    var dataCompanyB = { 
        id: 'company-b',
        name: 'Small Corp.',
        employees: []
    };

    // this extra instantiation is what causes the problem!!!!!!!!!!!!!!!
    var throwaway = new Company(dataCompanyA); 
    // but if the 'employees' HasMany relational in Company didn't have a reverseRelation, it wouldn't be a problem

    var companyA = new Company(dataCompanyA); 

    // init-ed a lead and its nested contacts are a collection
    ok(companyA.get('employees') instanceof Backbone.Collection, 'Company\'s employees should be a collection');
    equal(companyA.get('employees').length, 2, 'with elements');

    var collectionData = [dataCompanyA, dataCompanyB];
    var companyCollection = new CompanyCollection(collectionData);

    // after loading a collection with models of the same type
    // the existing lead should still have correct collections
    ok(companyA.get('employees') instanceof Backbone.Collection, 'Company\'s employees should still be a collection');

    // THIS TEST FAILS! (but shouldn't)
    equal(companyA.get('employees').length, 2, 'with elements');

});

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Aug 2, 2012

Owner

What is CompanyCollection? Just a generic one, like CompanyCollection = Backbone.Collection.extend({ model: Company }) I assume?

Owner

PaulUithol commented Aug 2, 2012

What is CompanyCollection? Just a generic one, like CompanyCollection = Backbone.Collection.extend({ model: Company }) I assume?

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Aug 2, 2012

Owner

Well, throwaway is created first, and is added to Backbone.Relational.store. When you create companyA later, an item with its id is already in it's store collection. Unfortunately, Backbone's new behavior is to fail silently in these cases instead of throwing an error. Options here:

  1. Throw an error ourselves. An error used to be thrown, but Backbone has changed to not throw an error when you try to add a duplicate item to a collection. Because errors annoy people, even when they're doing stuff they shouldn't ;).
  2. Keep the first model instance to claim an id; the current behavior.
  3. Keep the new model.

I'd say it should be 1, since this gives the most deterministic behavior. Otherwise, you're always going to have employees (or other relations) jumping ship when you don't expect them to.

Owner

PaulUithol commented Aug 2, 2012

Well, throwaway is created first, and is added to Backbone.Relational.store. When you create companyA later, an item with its id is already in it's store collection. Unfortunately, Backbone's new behavior is to fail silently in these cases instead of throwing an error. Options here:

  1. Throw an error ourselves. An error used to be thrown, but Backbone has changed to not throw an error when you try to add a duplicate item to a collection. Because errors annoy people, even when they're doing stuff they shouldn't ;).
  2. Keep the first model instance to claim an id; the current behavior.
  3. Keep the new model.

I'd say it should be 1, since this gives the most deterministic behavior. Otherwise, you're always going to have employees (or other relations) jumping ship when you don't expect them to.

@PaulUithol PaulUithol closed this in 351053c Aug 9, 2012

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Aug 9, 2012

Owner

Implemented #1, as this ensures a consistent and deterministic state for relations.

Owner

PaulUithol commented Aug 9, 2012

Implemented #1, as this ensures a consistent and deterministic state for relations.

@edwardmsmith

This comment has been minimized.

Show comment
Hide comment
@edwardmsmith

edwardmsmith Aug 12, 2012

This has broken our app.

I've not fully pinned down the issue, but its something like:

Package hasMany Assets

Fetching package returns an array of asset ids:

"assets": [
                "/v1/images/f4676c85e28344dd8968ab37fc005eda/",
                "/v1/images/fc104f6eda744ae7bc639fcc4b1757d2/",
                "/v1/images/c2f19c8afd59463d8d1bc9d475f18c3b/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/images/cf712eef86094eb09e46e6133d43a0f4/",
                "/v1/images/9cff2b64daf24ce7b3a06d1f0a456060/",
                "/v1/images/ab3f020e04a94c768aae785e208c58ac/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/"
            ]

In a view, I do

myPackage.fetchRelated('assets')

and the resulting call stack:

_.extend.register (dependencies.js:3405) (backbone-rel.js 344)
Backbone.RelationalModel.Backbone.Model.extend.set (dependencies.js:4317) (backbone-rel.js 1256)
Backbone.Model (dependencies.js:1832)
Backbone.RelationalModel.Backbone.Model.extend.constructor (dependencies.js:4118)
Asset (scripts.js:2078)
Backbone.RelationalModel.Backbone.Model.extend.build (dependencies.js:4527)
(anonymous function) (dependencies.js:4246)
_.map._.collect (dependencies.js:664)
Backbone.RelationalModel.Backbone.Model.extend.fetchRelated (dependencies.js:4237)
cmyk.ui.AssignmentMain.AssignmentMain._handle_fetch_new_package (scripts.js:6010)
__bind (scripts.js:5739)
_.extend.fetch.options.success (dependencies.js:1970)
f.extend._Deferred.e.resolveWith (dependencies.js:16)
w (dependencies.js:18)
f.support.ajax.f.ajaxTransport.send.d (dependencies.js:18)

Sorry about the line numbers - backbone-rel's built into a single JS file with other dependencies. I've added a couple reference points to the last two stack points.

I'll see if I can distill this down to a cleaner case.

edwardmsmith commented Aug 12, 2012

This has broken our app.

I've not fully pinned down the issue, but its something like:

Package hasMany Assets

Fetching package returns an array of asset ids:

"assets": [
                "/v1/images/f4676c85e28344dd8968ab37fc005eda/",
                "/v1/images/fc104f6eda744ae7bc639fcc4b1757d2/",
                "/v1/images/c2f19c8afd59463d8d1bc9d475f18c3b/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/",
                "/v1/images/cf712eef86094eb09e46e6133d43a0f4/",
                "/v1/images/9cff2b64daf24ce7b3a06d1f0a456060/",
                "/v1/images/ab3f020e04a94c768aae785e208c58ac/",
                "/v1/stories/9f534ac939544afa8b26e25fd6765e3d/"
            ]

In a view, I do

myPackage.fetchRelated('assets')

and the resulting call stack:

_.extend.register (dependencies.js:3405) (backbone-rel.js 344)
Backbone.RelationalModel.Backbone.Model.extend.set (dependencies.js:4317) (backbone-rel.js 1256)
Backbone.Model (dependencies.js:1832)
Backbone.RelationalModel.Backbone.Model.extend.constructor (dependencies.js:4118)
Asset (scripts.js:2078)
Backbone.RelationalModel.Backbone.Model.extend.build (dependencies.js:4527)
(anonymous function) (dependencies.js:4246)
_.map._.collect (dependencies.js:664)
Backbone.RelationalModel.Backbone.Model.extend.fetchRelated (dependencies.js:4237)
cmyk.ui.AssignmentMain.AssignmentMain._handle_fetch_new_package (scripts.js:6010)
__bind (scripts.js:5739)
_.extend.fetch.options.success (dependencies.js:1970)
f.extend._Deferred.e.resolveWith (dependencies.js:16)
w (dependencies.js:18)
f.support.ajax.f.ajaxTransport.send.d (dependencies.js:18)

Sorry about the line numbers - backbone-rel's built into a single JS file with other dependencies. I've added a couple reference points to the last two stack points.

I'll see if I can distill this down to a cleaner case.

@edwardmsmith

This comment has been minimized.

Show comment
Hide comment
@edwardmsmith

edwardmsmith Aug 12, 2012

Ah, my Assets array for the failing object has duplicates:

[
"/v1/stories/4fae89db76bd461993b53afcedb197d0/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/71f6948f85bd4171b4dab669e72846b2/",
"/v1/images/37ba750e9b454d358755e8b56f00bd13/",
"/v1/images/71f6948f85bd4171b4dab669e72846b2/",
"/v1/images/c705b5c6830244129539c9db9090b02b/",
"/v1/images/eea3a6623b7f4d63a00a652653d79113/",
"/v1/images/746ddcd1086e420da4285fda92237aa0/",
"/v1/images/37ba750e9b454d358755e8b56f00bd13/",
"/v1/images/c705b5c6830244129539c9db9090b02b/",
"/v1/images/eea3a6623b7f4d63a00a652653d79113/",
"/v1/images/746ddcd1086e420da4285fda92237aa0/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/71f6948f85bd4171b4dab669e72846b2/",
"/v1/images/37ba750e9b454d358755e8b56f00bd13/"
]

The issue with throwing an error is that in a case like this, I can't catch the error and continue fetching related.

So, may want to think about throwing the error a bit more - perhaps a warning. Or some option to not throw.

edwardmsmith commented Aug 12, 2012

Ah, my Assets array for the failing object has duplicates:

[
"/v1/stories/4fae89db76bd461993b53afcedb197d0/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/71f6948f85bd4171b4dab669e72846b2/",
"/v1/images/37ba750e9b454d358755e8b56f00bd13/",
"/v1/images/71f6948f85bd4171b4dab669e72846b2/",
"/v1/images/c705b5c6830244129539c9db9090b02b/",
"/v1/images/eea3a6623b7f4d63a00a652653d79113/",
"/v1/images/746ddcd1086e420da4285fda92237aa0/",
"/v1/images/37ba750e9b454d358755e8b56f00bd13/",
"/v1/images/c705b5c6830244129539c9db9090b02b/",
"/v1/images/eea3a6623b7f4d63a00a652653d79113/",
"/v1/images/746ddcd1086e420da4285fda92237aa0/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/1727e33984df4dafae7db0d25f8c084b/",
"/v1/images/71f6948f85bd4171b4dab669e72846b2/",
"/v1/images/37ba750e9b454d358755e8b56f00bd13/"
]

The issue with throwing an error is that in a case like this, I can't catch the error and continue fetching related.

So, may want to think about throwing the error a bit more - perhaps a warning. Or some option to not throw.

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Aug 12, 2012

Owner

Well, sounds to me like raising this error helped you discover an error in your data early, which could cause more serious (and harder to track down) problems down the line? I'd say that's a good thing.

However, I do agree we could be a bit more forgiving in fetchRelated. Or from the looks of things, it tries to load the same resource multiple times because its id occurs more than once in the relation? Maybe pulling _.uniq over that in fetchRelated would be a good idea regardless?

Owner

PaulUithol commented Aug 12, 2012

Well, sounds to me like raising this error helped you discover an error in your data early, which could cause more serious (and harder to track down) problems down the line? I'd say that's a good thing.

However, I do agree we could be a bit more forgiving in fetchRelated. Or from the looks of things, it tries to load the same resource multiple times because its id occurs more than once in the relation? Maybe pulling _.uniq over that in fetchRelated would be a good idea regardless?

@edwardmsmith

This comment has been minimized.

Show comment
Hide comment
@edwardmsmith

edwardmsmith Aug 14, 2012

A warning might have as well, but yes, you're right.

I do think that the situation with Backbone's not allowing multiple objects in a collection is not really analogous to the situation here because the collections in question in Backbone are ones that the user has total control over, and the collections in question in Backbone-Relational are ones that the user does not really have any control over.

I need to think about it a bit more and see if I can come up with any plausible use cases.

The change in fetchRelated might be a nice hardening thing.

edwardmsmith commented Aug 14, 2012

A warning might have as well, but yes, you're right.

I do think that the situation with Backbone's not allowing multiple objects in a collection is not really analogous to the situation here because the collections in question in Backbone are ones that the user has total control over, and the collections in question in Backbone-Relational are ones that the user does not really have any control over.

I need to think about it a bit more and see if I can come up with any plausible use cases.

The change in fetchRelated might be a nice hardening thing.

@philfreo

This comment has been minimized.

Show comment
Hide comment
@philfreo

philfreo Aug 14, 2012

Collaborator

#135 (comment)

@PaulUithol is the use cases described in the comment above related?

Collaborator

philfreo commented Aug 14, 2012

#135 (comment)

@PaulUithol is the use cases described in the comment above related?

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Aug 27, 2012

Owner

Re-opening; we should probably make fetchRelated resistant to there errors.

Owner

PaulUithol commented Aug 27, 2012

Re-opening; we should probably make fetchRelated resistant to there errors.

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Nov 28, 2012

Owner

The fix for #186 should cure the unfortunate by-effects this patch had on fetchRelated.

Owner

PaulUithol commented Nov 28, 2012

The fix for #186 should cure the unfortunate by-effects this patch had on fetchRelated.

@PaulUithol PaulUithol closed this Nov 28, 2012

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