Updated handling of multiple relationships between same records

[ajolipa]

In this PR

This is an initial attempt at addressing performant-software/core-data-cloud#509 by updating how we handle the case of multiple relationships between the same pair of records, so that the returned array will have only one entry for each related record, and when applicable the project_model_relationship_uuid field will contain an array of the UUIDs of all relationships between them.

Issues and questions

• Most obviously, this implementation leads to backwards compatibility issues since in the case of multiple relationships the type of the project_model_relationship_uuid field is different.
• I am honestly not sure how to make this play nicely with the project_model_relationship_inverse field...really in the case of multiple relationships that should also be an array, but I couldn't figure out how to get it to line up. In fact it seems like right now we're making the assumption that at least one of item.relationships and item.related_relationships is empty (see

  core-data-connector/app/serializers/concerns/core_data_connector/public/v1/typeable_serializer.rb

  Line 20 in d4a600c

  elsif !item.related_relationships.empty?

  ) so maybe this is the tip of the iceberg of the problem really...
• I couldn't figure out how to combine the ordering and the .distinct method, since the field we're ordering by is only in the joined table...I'm sure there's a way but this seemed to be just outside my ability to decipher Rails stuff.
• I think that it would be preferable to end up with separate items in the list of related records for each relationship, rather than an array of relationship UUIDs inside one record, but since the serializer seems to receive "related record" + "list of relationships" as its input I wasn't sure how to tell it to return the first relationship ID the first time and the second relationship ID the second time. Someone more fluent in Rails than me might be able to point me to where in the code that could be modified.

[blms]

I agree that there is a fundamental issue with the API for serializing related records here. It seems like what we should be doing is using a separate Relationship serializer, once per relationship. That would include the relationship's UUID, related type, UDFs on the relationship, etc. Then an endpoint like /people should be filtering that to just where the related type is Person.

Instead it seems like we are serializing each related Person record using the Person serializer class (for example). I see why, because we do want some/all of the fields from the related record's own serializer class, but it seems equally important that we get relationship record data serialized. Otherwise, there's no way to know that a record in the returned list is actually getting data like UUID and UDFs from the correct relationship with the primary record. You'd have to aggregate both into lists, and it wouldn't be clear which belong with which.

So yeah, I don't think /public/people and /public/people/:uuid/people should be using the same serializer. Instead it seems like the latter should use a serializer that serializes a Relationship instance, with a nested serializer (like has_one :related_record, polymorphic: true).

I suspect this involves changes to this API routing, where we want to resolve each of these to filtered lists of relationships rather than of related model instances. Might involve upstream changes in resource-api, I'm not sure. It's definitely going to be a breaking change either way—maybe a new numbered version of the API.

[ajolipa]

I agree that there is a fundamental issue with the API for serializing related records here. It seems like what we should be doing is using a separate Relationship serializer, once per relationship. That would include the relationship's UUID, related type, UDFs on the relationship, etc. Then an endpoint like /people should be filtering that to just where the related type is Person.

Instead it seems like we are serializing each related Person record using the Person serializer class (for example). I see why, because we do want some/all of the fields from the related record's own serializer class, but it seems equally important that we get relationship record data serialized. Otherwise, there's no way to know that a record in the returned list is actually getting data like UUID and UDFs from the correct relationship with the primary record. You'd have to aggregate both into lists, and it wouldn't be clear which belong with which.

This is very helpful and validating, thanks for taking a look! These are exactly my thoughts, put more eloquently. With the current setup using the Person serializer I couldn't wrap my brain around any way to track which item corresponds to which row of the relationships table. Having a separate serializer for the related records makes a lot of sense to me.

So yeah, I don't think /public/people and /public/people/:uuid/people should be using the same serializer. Instead it seems like the latter should use a serializer that serializes a Relationship instance, with a nested serializer (like has_one :related_record, polymorphic: true).

I suspect this involves changes to this API routing, where we want to resolve each of these to filtered lists of relationships rather than of related model instances. Might involve upstream changes in resource-api, I'm not sure. It's definitely going to be a breaking change either way—maybe a new numbered version of the API.

Okay that makes sense! Next question (maybe for @absempere or @jamiefolsom?) is what this means about dealing with the issue of duplicate related records appearing to be displayed on consuming apps, e.g. NBU, since they're launching in just over a month. Is it realistic to get this change implemented by then? Should I try to do some sort of temporary fix on the front end until this is fixed? (For example querying Typesense instead of the FairData API to get the relationship UUID.)