Representations that allow relationships between entities of the same kind

[andreacremaschi]

Hi again,

I have encountered another usage scenario that I thought it would work and it didn't..
It has to do with entities that can hold relationships to entity of their same kind (i.e. pages that link to subpages). In this scenario the correct reconstruction of the object graph depends on the order in which the objects representations appear.

Let's say we have a representation where objects and relationships are represented in this way and this precise order:

1. A    -> B, C
2. B
3. C

In the representation, every row contains an object representation and a representation of its relationships (let's say an array of ids).
What happens is that B and C proto records are first created when A is being parsed. When the parser consider row 2 it finds that B already exists in the recordResponseGroup, so it passes over. The issue here is that proto.dictionary now contains only the id of the object and not the full representation, and the marshaler will use this object representation instead of the full one present in row 2.

Of course the code works correctly if the representation is ordered like this:

1. B
2. C
3. A    -> B, C

The solution proposed in my PR checks the number of records in each representation and keeps the richest one. Is it too inelegant?

[cnstoll]

Very interesting!

This opens up all sorts of cans of worms. I've actually done this before in another project, in a previous version of MMRecord. The solution at the time basically populated the record multiple times, once per object contained in the response. It was pretty inefficient, and I removed it. I think this is worth supporting though, but I want to make sure we do it right.

There's a few ways of doing this that I see:

1. Your solution. Take whichever dictionary is bigger. May conceivably result in an incomplete population.
2. Merge all the dictionaries together into one. May conceivably result in conflicts where two representations of an object are not identical. This would tend to be an API issue though I would think.
3. Keep each dictionary and populate once per dictionary. This would increase the complexity a lot, which I am not a fan of.

Do you or anyone else reading this have any thoughts about that?

Thanks,

• Conrad

[andreacremaschi]

Well at first blush I would vote for the merging solution. I agree with your assumption that conflicts should be read as an API issue (representations should be atomic in time, so a same property can't have two different values), and I read 2) and 3) as different implementations of the same logical solution: 3) de facto would result in merging values from different dictionaries keeping the last value in the representation as the good one (other than being more complex to implement than just merging dictionaries). To complete this path of thought, the merging algorithm should perform "deep" merging (i.e. in the case we have two different representation of A, both exposing an array as a property containing different items) - keeping an eye on performance..

[cnstoll]

I think that keeping each dictionary and running multiple populates would effectively perform a deep merge, while the 2) might not necessarily do that unless it was performing a specific deep merge.

Deep merging would add a lot of complexity. I'm hesitant to support that out of the box, but would be interested in having that be possible through subclassing (not sure that it would be at the moment).

[cnstoll]

Pretty sure this PR that I just merged in will solve this problem :)

Cheers!

• Conrad

#76

[andreacremaschi]

Hey great! MMRecord is definitely becoming a quite sophisticated framework!

At the risk to become pedantic, I see a little but meaningful space for improvements here. Let's say a web service returns a dictionary containing two representations of the same entity, in this order: the first one contains the ID only, the second one contains the actual data. In your implementation the actual data would be ignored, unless the developer subclasses MMRecordMarshaler and provides its own implementation.

I would try to enforce your distinction between "mock" representation of objects containing primary keys only, and "fully saturated" objects to suggest a basic implementation for:

+ (void)mergeDuplicateRecordResponseObjectDictionary:(NSDictionary *)dictionary
                              withExistingProtoRecord:(MMRecordProtoRecord *)protoRecord

A fully saturated object should win on a mock one.. So the basic implementation should check if the protoRecord contains the ID only, or if it contains some actual data. In the first case, it would replace protoRecord's dictionary with the new value. In the second one it should do nothing.

[andreacremaschi]

See the PR: #78

[cnstoll]

Not pedantic at all. I need to look into this more, but I think this is actually a moot point at the moment. I am pretty sure that the default implementation actually will handle this case already. MMRecord should already distinguish between "mock" representations that consist only of primary keys, and real ones. For the "mock" ones, I'm pretty sure the population step is completely skipped, in favor of a "fetch the thing with this key, or if it doesn't exist, create an object with this key". I'm not 100% positive, but I believe this to be the case. I will look into it. Have you tested this to indicate something different?

By the way, if any of these configuration options or APIs feel unwieldy or like "overkill", please feel free to say so! Even though MMRecord is becoming fairly sophisticated, I still don't want it to become overly complicated :)

[cnstoll]

I stand corrected. It appears you are correct. In my tests, the first object does indeed win, which in some cases could be the single "I am just a primary key" instance of that object.

I'm going to take another stab at it and submit another PR. I think I'll essentially just provide a default implementation of the merge policy that will do essentially that. Stay tuned.

[cnstoll]

Want to take a look at my PR and let me know what you think? #79

This actually turns out to be a fairly complicated problem. I'm decently happy with this solution because it doesn't end up adding too much to the overall library, but this is definitely a deep rabbit hole.

This implementation will work very well for root-level objects returned in a response. Where it begins to break down a bit is with relationships inside of those objects.

If the reference to a relationship object inside of a fully saturated dictionary is an ID, and in the next case the relationship object is a real dictionary, its difficult to process the merge for those UNLESS you actually choose to really merge those two full-size dictionaries yourself. Here's an example:

[{id: foo, user: 53}, {id: foo, user: {id: 53, name: jeff}}]

In this case, the user would be correctly identified as 53 in both cases, AND even in my new implementation above, the merge method would be called the second time around for the new user. But, unless you choose to merge the full objects together, the new dictionary for the root level object will not contain (id: 53, name: jeff}, it will still contain just 53. :/

I'm hesitant to do anything about that by default.

However, the good news is that something like this should work:

[{id: foo, user: 53}, {id: bar, user: {id: 53, name: jeff}}]

In this case, the two top level objects are different, and they just happened to point to the same user. This time, when the user object is offered a chance to be merged, the second root level object will contain the fully saturated dictionary, and the user will be merged correctly.

Hope this all makes sense...If you can think of a non-ridiculous and generic way to handle the first case I am all ears, but I kind of doubt that there is one. The subclassing override point should at least make this situation able to handle by someone that needs to do it themselves.

Thanks,

• Conrad

[andreacremaschi]

This all makes perfectly sense.. your implementation seems fine to me and I think it will fit 90% of the cases, even though I can see your concerns. The more I think about the merge chunk of code the more specific cases come to my mind: i.e. the merge code makes the assumption that "primary key only" representation are dictionaries with one and only one key, but this is actually not guaranteed (i.e. some REST services send metadata together with the id to identify the type of the entity).
I think that your approach is correct: offering an override point for edge cases should make everybody happy. Here are my two cents:

1. Dictionaries containing only a primary key are often treated as valid "primary key only" representations:

[{id: foo, user: {id: 53}}, {id: bar, user: {id: 53, name: jeff}}]

Is this case covered in your implementation? I think it is.

1. Maybe I would try to detect edge cases in mergeDuplicateRecordResponseObjectDictionary and print a log once only when debugging asking the developer to take a look ("Hey the API you're talking to may be too complicated and requires your attention. Consider overriding mergeDRROD. This will be displayed just once").

Great work anyway!

[cnstoll]

I like the idea of adding some debug information to the merge method. I will do that.

Regarding the edge case of a single key in the dictionary, that actually IS a safe assumption...because of something that I should have documented better.

When a relationship on an entity is found MMRecord looks at the key in the entity's dictionary to see what it points to. If it points to a dictionary or an array (if its to-many) already, then...yay! Fantastic.

If the key points to a single value, either a string or number, it will create a new dictionary that contains that value AND uses the primary key indicated for the relationship as the key in the dictionary.

Through that means, we are guaranteed that all objects being merged will at least include a dictionary with at least one key/value pair :)

Yay!

Thanks,

• Conrad

👍