feat(fieldbehavior): ignore immutable in validation if not in field mask

[oscarmuhr]

This PR adds an additional check to the validation of IMMUTABLE fields to see whether or not the provided fieldmask is empty before checking if the immutable field is part of the message.

This to match the case in AIP-203 that states the service should ignore the field unless a change is requested. Previously, the validation would error any time the immutable field was included in the message, without regarding if an actual update was requested.

AIP-203
"When a service receives an immutable field in an update request (or similar), even if included in the update mask, the service should ignore the field if the value matches, but should error with INVALID_ARGUMENT if a change is requested"

[oscarmuhr]

The fieldmask in this test case should be completely empty. "" is also considered a value in most other cases in the aip-go library. Most importantly it aligns with this case in fieldmask.Update:

// Special-case: No update mask.
// Update all fields of src that are set on the wire.
case len(mask.GetPaths()) == 0:
	updateWireSetFields(dstReflect, srcReflect)
...

[radhus]

Your intention here is to treat empty update mask the same way as a wildcard, right?
If so, two thoughts:

I'm wondering if we should move the isEmpty logic to inside hasPath instead?

I think we should only treat empty update mask as wildcard if the update mask field is optional (not annotated with REQUIRED). WDYT?
Refer to AIP-134

The field may be required or optional. If it is required, it must include the corresponding annotation. If optional, the service must treat an omitted field mask as an implied field mask equivalent to all fields that are populated (have a non-empty value).

[oscarmuhr]

Yes, that is the intention.

1. It does seem to make sense, we would just have to judge the implications on the other fieldbehavior functions as well, but either way it would most likely make sense for them to have the same behavior.

2. Absolutely a valid point! If we decide to incorporate it into hasPath it would make sense there as well IMO.

[oscarmuhr]

@radhus After reviewing the code for this validation, the fieldmask.Update and fieldbehavior.Required validation i have some concerns:

1. [Done] It would be possible to add the isEmpty check to hasPath since the only other use of the function handles that case

// If no paths are provided, the field mask should be treated to be equivalent
// to all fields set on the wire. This means that no required fields can be missing,
// since if they were missing they're not set on the wire.
if len(mask.GetPaths()) == 0 {
	return nil
}

2. The library is consistently interpreting empty fieldmask as if it was wildcard, *, introducing the change in the library should probably be applied in a broader way and not just on one specific validation, WDYT?

3. The fieldbehavior of the fieldmask is linked to the FieldDescriptor on the parent message. With the way the function is implemented and how fieldmask works/is evaluated it is not required that the message passed as an argument is the actual request. It is possible today to use it with just the update value instead, something like this:

fieldbehavior.ValidateImmutableFieldsWithMask(
	req.GetShipment(),
	req.GetUpdateMask(),
)

As the use of an actual request message has not been enforced in any way, the evaluation of fieldbehavior_REQUIRED on the update_mask would have to be permissive to it not being included in the message parameter to the function or it would risk breaking the functionality. This would not fully defeat the purpose of the check, but it would somewhat dampen the effect it should have.

[radhus]

2. The library is consistently interpreting empty fieldmask as if it was wildcard, *, introducing the change in the library should probably be applied in a broader way and not just on one specific validation, WDYT?

Then I think make sense to keep being consistent and introduce a separate (potentially breaking) change later that respects the field behavior on the field mask, if we want that!

[radhus]

Looks good to me, nice! ⭐

[radhus]

2. The library is consistently interpreting empty fieldmask as if it was wildcard, *, introducing the change in the library should probably be applied in a broader way and not just on one specific validation, WDYT?

Then I think make sense to keep being consistent and introduce a separate (potentially breaking) change later that respects the field behavior on the field mask, if we want that!