fix(fieldmask): traverse repeated fields with wildcard in validation

[oscarmuhr]

fieldmask.Validate incorrectly allows targeting of subfields on repeated fields as

repeated.subfield

whereas AIP161 specifies that targeting of subfields MAY be added using wildcards as a suffix to the repeated field, such as:

repeated.*.subfield

This PR proposes to allow traversing repeated fields with the use of wildcard * when validating a fieldmask.

Note that the targeting subfields on repeated fields is not handled in fieldmask.Update.

...
case field.IsList(), field.IsMap():
	// nested fields in repeated or map not supported
	return
...

[odsod]

isn't the spec saying that it is supported on repeated fields, but the field in question must be a wildcard? *?

[odsod]

Field masks may permit the use of the * character on a repeated field or map to indicate the specification of particular sub-fields in the collection:

[oscarmuhr]

It does maybe the comment should be reworded, but it felt worthy of a clarification.
My intention was not to add the optional support for wildcard values in here but tackle the faulty interpretation of fieldmasks that don't traverse using wildcard *
The choice to disallow the use of these wildcards was then to bring it inline with how the fieldmask update function behaves where list traversal is not permitted. The AIP rule is also stated as you MAY support it, meaning we can stick to not supporting it.

I can look into adding the support to traverse with wildcard in both the validate and update functions if that would be something we want to add?

[oscarmuhr]

@odsod I have changed the PR to traverse repeated fields with wildcards to be fully aligned with AIP-161 and updated the PR description to explain that it does not align with how the fieldmask.Update function treats such paths in the fieldmask.

Looking into how to support this subfield targeting in the fieldmask.Update function, it does look relatively straight forward with something along these lines:

// a named field in a nested message
switch {
  ...
  case field.IsList():
    {
      if len(segments) > 2 && segments[1] == "*" {
        srcList := src.Get(field).List()
        dstList := dst.Get(field).List()
        if srcList.Len() != dstList.Len() {
          // TODO: What to do if repeated fields are not the same length?
        }
        for i := 0; i < srcList.Len(); i++ {
          updateNamedField(dstList.Get(i).Message(), srcList.Get(i).Message(), segments[2:])
        }
      } else {
        return
      }
    }
  ...

As illustrated in the snippet, there is a question not addressed in AIP-161 with how lists of different length should be handled. Without breaking the current implementation of fieldmask.Update I see the following options:

• Skip traversing if lists are not the same length (updates are susceptible to target unintended repeated elements)
• Traverse the src list for all elements up until dstList.Len() (avoid adding new elements into the repeated field)
• Add new elements containing only fields targeted by the fieldmask (risks introducing data that is inconsistent with regards to required fields etc...)
• Panic (not a good option imo given that this is very likely to happen in a runtime environment as opposed to earlier panic on comparing different message types)

I think that neither of these options are particularly great so maybe it is fine that fieldmask.Validate and fieldmask.Update methods diverge in handling of repeated fields? It is just a little bit confusing if that is the case.

If we could update the method signature of fieldmask.Update to return an error (or introduce another method that can handle targeting of subfields on repeated fields) this would be the better way for this method to support AIP-161 targeting of repeated fields fully in the aip-go library.