AIP-121: Immutable field produces "mutable resource reference introduces a reference cycle"

[loeffel-io]

Hey @noahdietz and @quirogas

i think i found an issue 🤔

This project still runs v2.0.0 but should be still present in v2.3.0

message UserEntitlement {
  option (google.api.resource) = {
    type: "billing.mindful.com/UserEntitlement"
    pattern: "projects/{project}/users/{user}/entitlements/{user_entitlement}"
    singular: "userEntitlement"
    plural: "userEntitlements"
  };
  // The resource name.
  // Format: projects/{project}/users/{user}/entitlements/{user_entitlement}
  string name = 1 [
    (google.api.field_behavior) = IDENTIFIER,
    (buf.validate.field).string = {pattern: "^projects\\/[a-z]([a-z0-9-]{0,61}[a-z0-9])?\\/users\\/[a-zA-Z0-9-]{1,128}\\/entitlements\\/[a-z]([a-z0-9-]{0,61}[a-z0-9])?$"},
    (buf.validate.field).ignore = IGNORE_IF_ZERO_VALUE
  ];
  // The previous entitlement.
  // Format: projects/{project}/users/{user}/entitlements/{user_entitlement}
  string previous_entitlement = 2 [
    (google.api.field_behavior) = IMMUTABLE, <-------------------------------
    (google.api.resource_reference) = {type: "billing.mindful.com/UserEntitlement"},
    (buf.validate.field).string = {pattern: "^projects\\/[a-z]([a-z0-9-]{0,61}[a-z0-9])?\\/users\\/[a-zA-Z0-9-]{1,128}\\/entitlements\\/[a-z]([a-z0-9-]{0,61}[a-z0-9])?$"},
    (buf.validate.field).ignore = IGNORE_IF_ZERO_VALUE
  ];

  problems:
    - message: |-
        mutable resource reference introduces a reference cycle:
        billing.mindful.com/UserEntitlement > billing.mindful.com/UserEntitlement
      location:
        start_position:
            line_number: 150
            column_number: 5
        end_position:
            line_number: 150
            column_number: 83
        path: mindful/earth/billing/user/v1/user_entitlement.proto
      rule_id: core::0121::no-mutable-cycles
      rule_doc_uri: https://linter.aip.dev/121/no-mutable-cycles

[noahdietz]

Hey @loeffel-io ! @quirogas and I chatted about this, he will follow up.

Thanks for your patience, sorry for the slow response, it's been busy!

[quirogas]

Hi @loeffel-io,

Thank you for raising this issue and providing a detailed example!

I believe the linter is behaving correctly. The core issue is that while the previous_entitlement field is marked as IMMUTABLE, this doesn't prevent a potential cycle from being formed when the resource is initially created. AIP-121 states that relationships between resources (via resource references) "MUST be representable via a directed acyclic graph." This is to avoid complexities in managing resource lifecycles (creation, deletion, updates), and it explicitly provides an exception to the acyclic requirement only for fields marked as (google.api.field_behavior) = OUTPUT_ONLY. This is because OUTPUT_ONLY fields are server-managed, so the client doesn't bear the complexity of resolving/managing cycles. However, IMMUTABLE means the field can be set by the client once during the creation of the resource, but not changed thereafter. In your example, since the client provides the previous_entitlement value upon creation, they could potentially specify a reference that forms a cycle.

The linter rule core::0121::no-mutable-cycles flags this because the UserEntitlement resource contains a field (previous_entitlement) that can reference another resource of the same type (billing.mindful.com/UserEntitlement). This creates a cycle in the resource type graph. The simplest example of a problematic cycle that can be formed during creation (and which IMMUTABLE doesn't prevent) is a self-reference. While you might have business logic preventing this, the proto definition itself allows it. AIP-121 aims for the schema to represent an acyclic graph of dependencies for user-settable fields.

That being said, I believe your example is valid, but it requires custom logic beyond the baseline assumptions and guardrails that the AIPs aim to establish for simpler, more consistent APIs. Finally, here what I think we could do to address this:

1. We could add a note in the rule that explain why IMMUTABLE still allows cycles.
2. We could add a check in the rule that provides a better message -- I feel like the use of the word mutable may lead developers into using IMMUTALBE naturally.
3. My analysis is flawed, at which point we could simply add a check to the linting rule so it doesn't trigger when the field is IMMUTABLE, but I would need to be convinced.

I'm leaning towards options 1 and 2, let me know what you think :)

[loeffel-io]

thanks @noahdietz and @quirogas! sorry for the late response, my hand was disabled..

i completely agree and i will use OUTPUT_ONLY since the server can simply pick the previous entitlement itself and this should not be handled manually.

leaning towards option 1 and 2 sounds good!