dchester / epilogue

Create flexible REST endpoints and controllers from Sequelize models in your Express app
846 stars 116 forks source link

Association not correct after UPDATE #97

Closed xmakro closed 9 years ago

xmakro commented 9 years ago
epilogue.resource({
    model: db.Task,
    include: {
        model: db.User,
        as: "owner"
    }
});

After using a PUT /task/:id request to update the ownerId of a task object, the populated owner object in the json response points to the old owner, not the newly updated one.

mbroadst commented 9 years ago

hey, what database are you working with?

xmakro commented 9 years ago

I am working with mysql. Should I switch?

mbroadst commented 9 years ago

No no I just wanted to check since there are known issues with some databases and returning values. This is strange since as you can see here we default to reloading instances after these operations.

If the result of those two tests is as expected, then this might just be an incompatibility with sequelize and we'll have to dig deeper. Would you be willing to help put together a test case for this? I think that's going to be the fastest way to solving your problem.

xmakro commented 9 years ago

Sure no problem. The following is an actual response from the server:

{
  "id": 113,
  "title": "simple example task",
  "userId": 1,
  "ownerId": 5,
  "owner": {
    "id": 2,
    "name": "Testuser"
  }
}

I can confirm that the put really worked and there was a change in the database of the ownerId from 2 to 5. Still the populated object is the old owner.

I checked this line but it is never called because reloadAfter is false. I investigated this further and the reason for the bug is as follows:

In this line the associations are checked. In my example, Task has two associations to the User model through userId and ownerId. However, self.resource.associationsInfo is an object with the model name as a key, so each model can only be associated once. This would look as follows:

 { user: { identifier: 'userId', primaryKey: 'id', as: 'user' } }

When self.resource.associationsInfo should really look something like this:

 { 
     ownerId: { identifier: 'ownerId', primaryKey: 'id', as: 'owner' } },
     userId: { identifier: 'userId', primaryKey: 'id', as: 'user' } }
  }

So instead of using the model.name as a key, the identifier of the association should be used.

mbroadst commented 9 years ago

sounds legit, but I won't have time to work on this for another day or so. Maybe you could take a look at the existing associations tests and come up with a test that breaks this? It shouldn't be too too hard, that will probably make the fix incredibly easy

xmakro commented 9 years ago

i can look into it on the weekend

xmakro commented 9 years ago

http://pastebin.com/qa6qXUvH

This is basically the original test, except that User has a second address field that is defined before the original address field. This causes the put test to fail for the explained reason in the post above. As you see the test does not fail in line 164, but in line 165 where the address object is not updated.

mbroadst commented 9 years ago

Thanks for the test case. So the original title of this issue was a bit of a misnomer, but the real issue is that we only cached thefirst relevant association which had a target of each included model in a resource (phew that was a mouthful). I've fixed that in 3b3e6697ce03fce5a64bbee8bf7abd1586a81156, please give it a whirl

xmakro commented 9 years ago

Thanks for the fix, works great now!