fix(AIP-4231): include type and child_type ref compat guidance

[noahdietz]

This backfills guidance that wasn't written down explicitly, but should've been implemented in all languages that support resource references.

Covers two cases that involve switching between type and child_type references that should be safe to make as the the field either remains the same or gains additional possible patterns.

[jskeet]

This looks correct to me, but I'll need to take a bit of time to check that we actually comply with it in C#...

[noahdietz]

This looks correct to me, but I'll need to take a bit of time to check that we actually comply with it in C#...

SGTM! Holding, no real rush.

[jskeet]

Tested for C# with Cloud Functions v2. In ListFunctionsRequest:

   string parent = 1 [
     (google.api.field_behavior) = REQUIRED,
     (google.api.resource_reference) = {
       child_type: "cloudfunctions.googleapis.com/Function"
     }
   ];

I changed the child_type: ... to type: "locations.googleapis.com/Location" and regenerated: the generated source code did not change at all (only the proto descriptor bytes, which is expected).

[noahdietz]

I changed the child_type: ... to type: "locations.googleapis.com/Location" and regenerated: the generated source code did not change at all (only the proto descriptor bytes, which is expected).

Wonderful. Thanks for checking.

Do you want to check the type to child_type expansion case as well or do we know that works?

[jskeet]

Do you want to check the type to child_type expansion case as well or do we know that works?

Hmm... I think this shows it works when we go from type to a child type that only has a single parent.
It may be breaking if you go from (say) Project to parent-of-SomeResource when SomeResource could be a Project, Organization or Folder.

It would definitely be a semantic breaking change, in that you'd be going from saying "this value is always in format X" to "this value is always in format X, Y or Z" (so anyone assuming X would be broken). Hmm.

(This may also depend on the name of the field, at least in C#. I'm not sure how far we want to go with this.)

[noahdietz]

It would definitely be a semantic breaking change, in that you'd be going from saying "this value is always in format X" to "this value is always in format X, Y or Z" (so anyone assuming X would be broken). Hmm.

I think my perspective is a bit skewed towards request message resource_references, where the user sets the value, and as long as what they were setting previously is still supported then adding new supported patterns isn't breaking.

I think you are right that it would be risky in the response case, where user assumes it is always X, but suddenly it's Y...that said I don't know what level of support we have for references in responses since we don't typically control the response message (protobuf codegen does), whereas with request fields, we control the generated function signature at the very least.

[jskeet]

Yes, it's definitely simpler for request messages. I'll try to work out a way of testing the "specific type to child_type with extra parents" situation. It may be a matter of making a breaking change in one commit (that never gets published) and then checking whether going back again is not breaking...

[jskeet]

Okay, testing with https://github.com/googleapis/googleapis/blob/master/google/cloud/accessapproval/v1/accessapproval.proto

First, change ListApprovalsRequestMessage.parent to:

string parent = 1 [(google.api.resource_reference) = {
  type: "cloudresourcemanager.googleapis.com/Project"
}];

Regenerate and commit. This commit is effectively the reverse of what we want to do, but it's expected to be breaking. The interesting bit is when we revert the change in the proto, so converting it to:

string parent = 1 [(google.api.resource_reference) = {
  child_type: "accessapproval.googleapis.com/ApprovalRequest"
}];

Regenerate and commit.

The commit can be seen here (for now - I'll delete the branch in a bit).

Changes are:

• Additional overloads for ListApprovalRequests, e.g. ListApprovalRequests(gagr::FolderName parent, string pageToken = null, int? pageSize = null, gaxgrpc::CallSettings callSettings = null)
• Additional properties on ListApprovalRequestsMessage of ParentAsFolderName, ParentAsOrganizationName, ParentAsResourceName

So that particular example looks compatible.

Maybe we should be slightly more coy about this and say that changes to request message resource reference fields should be compatible?

[noahdietz]

Thank you for testing all of this!

Maybe we should be slightly more coy about this and say that changes to request message resource reference fields should be compatible?

I'm fine with that :) PTAL I only changed that second case of type to child_type to be scoped to request references.

[jskeet]

Yup, that looks good :)

[blakeli0]

I wonder if we could provide examples regarding what single-parented means, or links to other AIPs if it is already explained? They may seem straightforward to people that are familiar with the concept of resources, but could take a while for people to figure it out by themselves if they are not.

[noahdietz]

Fair point, the distinction is actually described earlier in this AIP under the heading Multi-pattern resources. Would you still like to see some reference here?

[noahdietz]

I've updated the language to be a little more specific regarding the parts of the resource definition that are pertinent to evaluation, as well as references to the relevant sections for more examples.

[noahdietz]

@blakeli0 thanks for reviewing, I was about to ask for your thoughts :D

[blakeli0]

Maybe is the only matching parent? Because if a child has multiple parents, it would not be backwards-compatible.

If I'm understanding correctly, if the child type is

option (google.api.resource) = {
    type: "recaptchaenterprise.googleapis.com/RelatedAccountGroupMembership"
    pattern: "projects/{project}/relatedaccountgroups/{relatedaccountgroup}/memberships/{membership}"
  }

It has a single pattern and Project is actually one of the matching parent, but changing the child_type to Project would still be breaking because it is not the only parent.

[noahdietz]

Project isn't the parent (though I guess you could call it a grandparent) because that's not longest matching pattern sans child resource's segments - /memberships/{membership}

It sounds like you want more clarity on such definitions, but I don't think this is the right section to do that in - this language needs to be concise and succinct regarding compatibility.

Can we instead improve such documentation in a separate PR?

[blakeli0]

SGTM. I definitely want to understand more about how the parent of child_type is resolved, it would be great if we have examples for it. Back to my question though, is it possible for a child_type to have multiple parents if it only has one pattern?

[noahdietz]

For sure, you got it!

Back to my question though, is it possible for a child_type to have multiple parents if it only has one pattern?

Nope! I will start by saying that the current google.api.resource annotation does not express this well, which is why you and I are here and why we need to add some words to fix it :)

The basic form of a resource name pattern is roughly expressed in regex as

(?<parent>([a-z][a-zA-Z0-9]*\/\{[a-z][_a-z0-9]*[a-z0-9]\}\/)*)(?<child>[a-z][a-zA-Z0-9]*\/\{[a-z][_a-z0-9]*[a-z0-9]\})

Plugging that into regex101 you get this: https://regex101.com/r/N9fbQ7/1 - take a look at how the parent and child named groups change as the pattern gets longer.

Basically, a resource name is multiple pairs of pluralCollectionName/{singular_typ_name} over and over, with the last pair being the "child" resource collection, and the concatenation of all previous pairs being the "parent" resource.

Put differently, if I define a resource Foo with pattern foos/{foo}, then a child Bar with foos/{foo}/bars/{bar}, and another Baz with foos/{foo}/bars/{bar}/bazs/{baz} and I want the parent of Baz, the resource that defines the name pattern with the summation of all preceding pairs is Bar