search

Rightpoint / RZVinyl

Stack management, ActiveRecord utilities, and seamless importing for Core Data
Other
21 stars 6 forks source link

cull duplicates in array imports #48

Closed mr-fixit closed 9 years ago

mr-fixit commented 9 years ago

If you import the same object (as defined by 'A.primary-key == B.primary-key') in two separate operations, you can expect the values from B to modify the existing object, A.

When importing an array, the check for 'same primary-key' was using only the objects that existed before the array started getting imported. If the array being imported contained multiple objects with the same key, all of them would be imported.

This fix unmasked another bug, in test_BigImport_1000. The test was importing multiple artists, all owning the same song, and expecting the song to belong to all of them. What was happening is that, due to the to-one-ness of the song's relationship to its artist, the song ended up being owned by the last artist imported, all other artists ended up owning no songs. The test was checking [array lastObject], which may not have been the last song imported. I'm don't know why the test worked before, or why my fix unmasked this bug, I'd be happy to have someone explain it to me.

cmds4410 commented 9 years ago

Just ran into this one too! Going to try out your branch to see if it fixes the issue on my end.

cmds4410 commented 9 years ago

It fixes it. :)

mr-fixit commented 9 years ago

Lemme know if my input is needed, otherwise I'm just going to be happy that my PR is getting looked at.

cmds4410 commented 9 years ago

@mr-fixit Happy to look :) I'm actually still seeing a duplicate; doing some investigating on my end.

cmds4410 commented 9 years ago

@mr-fixit This seems to be working great for me! The one case I've found that it doesn't cover (understandably) is when you're importing an array of items where one of the top-level items contains other top-level item as a child.

For example: [{name: item1, parent: item2, children: nil}, {name: item2, parent:nil, children:[{name: item1, parent: item2, children: nil}]}]

I think this can/should probably be solved in the backend or by filtering this data manually (fwiw I'm currently doing the latter). In order to tackle this in RZVinyl, we'd probably need to run a linear, O(n) check on the the final imported array to find and delete any duplicates. I suppose that could be an interesting option for people using APIs that behave this way. Or maybe I'm missing something obvious?

jwatson commented 9 years ago

Is this good to merge, or do we want to address the edge case @cmds4410 mentioned? Can you give a 2nd opinion @KingOfBrian? Mine is merge it.

mr-fixit commented 9 years ago

I say "merge it". Don't let perfect be the enemy of better.

KingOfBrian commented 9 years ago

Yea, looks good to me! @jwatson

cmds4410 commented 9 years ago

I agree, yay!