Add references parameter to relationships

[ellisandrews-toast]

Summary

Adds a references parameter to relates_to_one and relates_to_many that allows specifying which field the foreign key points to, instead of always assuming id. Defaults to "id" for backward compatibility.

# Before (implicit: checkGuid points to Check.id, still supported syntax)
t.relates_to_many "costOfGoods", "CostOfGoods", via: "checkGuid", dir: :in

# After (explicit: checkGuid points to Check.guid)
t.relates_to_many "costOfGoods", "CostOfGoods", via: "checkGuid", references: "guid", dir: :in

Changes

• relates_to_one and relates_to_many accept an optional references: keyword (default "id")
• The value is stored on Relationship, passed through runtime metadata, and used by RelationJoin at query time
• register_inferred_foreign_key_fields uses references instead of hardcoded "id"
• Added validate_references in RelationshipResolver to validate the field exists (for :out) and is an ID type
• Test schema updated with Manufacturer.guid and parallel relationships exercising both directions

[myronmarston]

Instead of adding a new relationship--which isn't exercised by any acceptance tests to demonstrate that references works end-to-end--can we update some existing relationships? In particular:

• Let's find relationships that are used by both GraphQL acceptance tests and indexing source_from relationships in acceptance tests, and update them
• Let's cover both a _one and _many relationship
• Let's cover both a dir: :in and a dir: :out relationship

...maybe even cover the combination those dimensions.

Here's what I'm concerned about: you've demonstrated via this test schema change that references: shows up in the runtime metadata as you expect, but you haven't demonstrated that it works for GraphQL relationships or for sourced_from indexing relationships.

I don't think we need to retain any relationships that rely on the default of "id"--as I see it, if a feature works end-to-end for references: "arbitrary_string" then we can trust it'll work for the default of "id", too. But the opposite is not true: there may be behavior that works for "id" that does not work for an alternate references: value due to us hardcoding "id" in some spots that you missed. Updating relationships to use a specific references option (with a different value than "id") is a forcing function to make sure we've updated all the spots that hardcoded "id" before.

So I guess one option is we could update every relationship to use an alternate reference field but that's probably overkill--just make sure we've covered examples of all the cases so we're sure that all code paths are covered.

[myronmarston]

It's concerning that you updated this file without changing any unit, integration, or acceptance tests in elasticgraph-graphql. That means that someone could revert these to the hardcoded "id" in the future and our test suite wouldn't catch it!

With my suggestion on config/schema/widgets.rb to update the existing relationships, we should gain a bit of automatic acceptance test coverage of this. In addition, it would be good to update https://github.com/block/elasticgraph/blob/main/elasticgraph-graphql/spec/integration/elastic_graph/graphql/resolvers/nested_relationships_spec.rb to exercise this. Similar to my request above, it would be good to cover the various cases (in/out, many/one, etc) with a references: override to prove it works.

[ellisandrews-toast]

Here's the current state of this PR (which I know is failing CI, despite passing scripts/quick_build locally):

I ran into a bunch of issues trying to make this references: param work generically for relationships. In a sentence, this is due to the assumption in the framework that all indexed types have a field called id which must be the document _id in Elasticsearch/OpenSearch.

When you have a relationship between indexed types:

Widget.relates_to_one "manufacturer", via: "manufacturer_id" ...

ElasticGraph assumes this flow:

1. The manufacturer_id foreign key contains the value from Manufacturer.id
2. That same value became the document _id when the Manufacturer was indexed
3. Queries use manufacturer_id to filter: {id: {equal_to_any_of: [manufacturer_id]}}
4. Updates use manufacturer_id as the document _id to locate the Manufacturer document

i.e. foreign key value == id field value == document _id value

It would be a pretty big change I think to change this assumption everywhere it is relied upon.

With that said, I messed around a little with embedded (i.e. not indexed) types, because embedded types don't have document _id values - they're just nested objects within their parent documents. So the references concept would work for some of those cases where you just needed to resolve a relationship by matching on a value.

I tried to show this working by adding some new types to the example Widgets schema and adding a graphql acceptance test for it. Obviously not ideal to add a bunch of models just to show this, but no existing relationships covered what I wanted to see for this. And I'm doubtful that this feature in this state would be acceptable anyway so I'm not exactly proposing adding those to the schema on main.

Overall, it feels kind of hacky to me to add all this code to only support configurable references for this small subset of relationships though (embedded types that have a relationship to an indexed type), so I'm probably going to just table this. But the code is up if it's interesting.

[myronmarston]

It would be a pretty big change I think to change this assumption everywhere it is relied upon.

At the end of the day, I'm in favor of removing this assumption, if we can do it safely and correctly. As I see it, id is purely a naming convention (although it is recommended at the GraphQL site). I think it's a good default but I don't think it's fundamental or important to EG to allow alternate ID fields.

But I think that to remove the assumption, this will have to be broken down into smaller sub-problems that we can tackle individually. It's too much to tackle in one PR.

Overall, it feels kind of hacky to me to add all this code to only support configurable references for this small subset of relationships though (embedded types that have a relationship to an indexed type), so I'm probably going to just table this. But the code is up if it's interesting.

Agree that would be hacky. If we're going to support references, it needs to be fully supported, IMO.

For now I've converted this back to draft.