AIP-121: lint against cyclic references

[toumorokoshi]

AIP-121 states:

The relationship between resources, such as parent-child or resource references, must be representable via a directed acyclic graph.

That should be added.

[noahdietz]

This rule is necessary for a candidate in CS-2312, so I'm bumping the priority.

[noahdietz]

I'm trying to nail down the concrete definition of a cycle. There are probably a few scenarios.

1. Parent with child reference annotated as required - impossible to create valid Parent without Child reference, but Child can't be created without a Parent to create with:

message Parent {
  // ...
  string child = 2 [
    (google.api.resource_reference).type = ".../Child",
    (google.api.field_behavior) = REQUIRED
  ];
}

2. Parent with any child reference, regardless of field_behavior - requires the "create parent, create child, update parent" cycle

message Parent {
  // ...
  string child = 2 [
    (google.api.resource_reference).type = ".../Child"
  ];
}

Solving for 2 solves for 1 as well, obviously. Though I wonder, would an output_only reference to a child be OK? I think so. Which means only references that are accepted as input are in scope for cycle checks.

3. Resource A that refers to Resource B, which refers back to Resource A

message A {
  // ...
  string b = 2 [
    (google.api.resource_reference).type = ".../B"
  ];
}

message B {
  // ...
  string a = 2 [
    (google.api.resource_reference).type = ".../A"
  ];
}

This is hard to tell if its even bad because just having the reference cycle does not mean that it will be a cycle at runtime i.e. Resource A instance 123 refers to Resource B instance abc and vice versa. But I think this might be an example of the issue described in the AIP The delete operation may also become more complex. Need some clarity on this - how strict are we really trying to go?

[toumorokoshi]

Though I wonder, would an output_only reference to a child be OK? I think so.

yep, agreed. It's only mutable references.

This is hard to tell if its even bad because just having the reference cycle does not mean that it will be a cycle at runtime i.e. Resource A instance 123 refers to Resource B instance abc and vice versa.

I think the precise concern is that cylic references result in multi-phase operations, touching at least one of the resources twice.

I'd say scenario three (A has a mutable reference to B, b have a mutable reference to A) is also invalid.

But I think this might be an example of the issue described in the AIP The delete operation may also become more complex. Need some clarity on this - how strict are we really trying to go?

Yes - the delete operation may require unlinking the resources first (e.g. removing the resource references), then performing the delete afterward on each. This isn't strictly required as it depends on the behavior of the resource, but worst-case it'll require this multi-phase work.

I think we should ensure that no mutable references are cyclic. So among the examples above 1,2,3 are all invalid. If there was a fourth example with an OUTPUT_ONLY cyclic reference, that would be valid.

I'd also note that similar to the field_behavior rule, we'd probably have to only check the current package, avoiding some situation where a service team can't actually fix the issue by themselves.

[noahdietz]

Thanks that clarifies things a lot.

Is it worth following up in the AIP with a fix to make this distinction?

[toumorokoshi]

there's a clause around output_only fields, but maybe it should be updated to be more like the AIP must: https://google.aip.dev/121#cyclic-references

[noahdietz]

Ok great, I've got the rule PR up