fieldmask: use field behavior for applying updates

[nlachfr]

Hello,

The AIP-203 specification defines custom behavior for updates using field masks :

• a field with (google.api.field_behavior) = OUTPUT_ONLY should be ignored
• a field with (google.api.field_behavior) = IMMUTABLE should raise an error when the field is modified

At the moment, these behaviors are not taken into account by the fieldmask module and I think that it would be nice if it was supported.

What do you think about adding it in the fieldmask.Update(mask *fieldmaskpb.FieldMask, dst, src proto.Message) function ?

[vallahaye]

Hello everyone,

Just a friendly comment to revive this issue :eyes: Implementing this feature would allow us to write fully AIP-compliant update methods very easily like so:

func (s *iamSvc) UpdateUser(ctx context.Context, in *iamv1.UpdateUserRequest) (*iamv1.User, error) {
  // You must first validate and authorize the request but it is out of scope
  now := timestamppb.Now()
  user, err := s.usersRepo.GetUser(ctx, in.User.Name)
  if err != nil {
    return nil, err
  }
  if err := fieldmask.Update(in.UpdateMask, user, in.User); err != nil {
    // An error is thrown when an IMMUTABLE constraint is violated
    return nil, errors.Wrap(err, errors.CodeInvalidArgument, "invalid update mask")
  }
  // user.CreateTime is not updated because of the OUTPUT_ONLY constraint
  user.UpdateTime = now
  if err := s.usersRepo.SaveUser(ctx, user); err != nil {
    return nil, err
  }
  return user, nil
}

With very little changes you can transform the method above into an upsert:

func (s *iamSvc) UpdateUser(ctx context.Context, in *iamv1.UpdateUserRequest) (*iamv1.User, error) {
  // You must first validate and authorize the request but it is out of scope
  now := timestamppb.Now()
  user, err := s.usersRepo.GetUser(ctx, in.User.Name)
  if err != nil {
    if errors.CodeOf(err) != errors.CodeNotFound || !in.AllowMissing {
      return nil, err
    }
    user = in.User
    user.CreateTime = now
  } else {
    if err := fieldmask.Update(in.UpdateMask, user, in.User); err != nil {
      // An error is thrown when an IMMUTABLE constraint is violated
      return nil, errors.Wrap(err, errors.CodeInvalidArgument, "invalid update mask")
    }
    // user.CreateTime is not updated because of the OUTPUT_ONLY constraint
  }
  user.UpdateTime = now
  if err := s.usersRepo.SaveUser(ctx, user); err != nil {
    return nil, err
  }
  return user, nil
}

As you can see, the new behavior of the fieldmask.Update function makes the code very easy to write and understand.

Hoping to see this implemented one day, I wish you a great day.

[DavyJ0nes]

Hey there, picking up on this thread.

Reading through the field behaviour AIP about immutable fields it says:

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.

To me this means that the decision is left up to the service author to either ignore those fields or throw an error if a caller provides them.

With this in mind I'd like to add a new validation function to check if immutable fields are set during an update, similar to fieldbehavior.ValidateRequiredFieldsWithMask. This will allow service authors to validate the request and fail early but also not introduce a breaking change to the fieldmask.Update function.

What do you think?

[DavyJ0nes]

I put up a PR to implement the new validation function in #180

[github-actions[bot]]

This issue has been open for 365 days with no activity. Marking as stale.