protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

Why are repeated fields only allowed at the last position of a paths string?

Open nnutter opened this issue 4 years ago • 5 comments

https://github.com/golang/protobuf/issues/1179 broke us and I don't understand why this constraint exists. It noted,

field_mask.proto mentions that ...

// Field masks are used to specify a subset of fields that should be
// returned by a get operation or modified by an update operation.

...

// # Field Masks in Projections
// ...
// A repeated field is not allowed except at the last position of a
// paths string.

...

// # Field Masks in Update Operations
// ...
// Note that a repeated field is only allowed in the last position of a `paths` string.

Given those statements, I think FieldMask.IsValid should return false if a path contains a repeated field that is not in the last position.

Given the example in https://github.com/golang/protobuf/issues/1179 why is nests.s not a reasonable path? Imagine extending Nested with a second field. It seems reasonable to say you want one of those fields but not both.

nnutter avatar Apr 30 '21 18:04 nnutter

You'll need to bring this up on the main Protocol Buffers site at https://github.com/protocolbuffers/protobuf/issues as the Go implementation is simply following the guidelines mentioned in the proto definition.

For a repeated field to not be in the last position, I think they may need to also define a way to reference a specific item (or items) in the repeated field, which they currently don't. It's possible that it implies that a subfield applies to all items, but again I prefer not to speculate as there is no specification for this.

cybrcodr avatar Apr 30 '21 19:04 cybrcodr

Hi @nnutter, I'm sorry to hear that you were broken by the change. The purpose of this project is to comply with the semantics defined by the main protocol buffer project. Since the documentation of field_mask.proto explicitly calls out rejecting repeated fields not in the last position, it seems we're compelled to comply. @cybrcodr is right that the right place to bring this up is at https://github.com/protocolbuffers/protobuf/issues if you think the semantics should be loosened.

dsnet avatar Apr 30 '21 20:04 dsnet

Thanks @dsnet and @cybrcodr. I've created a feature request over there, https://github.com/protocolbuffers/protobuf/issues/8547.

nnutter avatar May 01 '21 01:05 nnutter

Cross-posting this on the upstream issue and re-opening.

A coworker noticed AIP-161: Field masks: Wildcards is accepted as of March and satisfies this issue. What is the process it will take to get from AIP to defined here?

For convenience/reference here are some excerpts:

  • AIP-161: Field masks: Map fields says,

    Field masks may permit the specification of specific fields in a map, if and only if the map's keys are either strings or integers, using the . character for traversal.

    // Valid field masks: reviews, reviews.smith, reviews.`John Smith`
    map<string, string> reviews = 2
    
  • AIP-161: Field masks: Wildcards says,

    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

    // Valid field masks: authors, authors.*.given_name, authors.*.family_name
    // Invalid field masks: authors.0, authors.0.given_name
    repeated Author authors = 2;
    

nnutter avatar Aug 10 '21 15:08 nnutter

This still needs to be addressed at https://github.com/protocolbuffers/protobuf/issues as the documentation for field_mask.proto is hosted there. AIP documentation is considered to be a lesser degree authority than the field_mask.proto documentation itself. The AIP certainly seems in contradiction to the documentation at field_mask.proto and it is the responsibility of the main protobuf project to reconcile those issues.

dsnet avatar Aug 10 '21 16:08 dsnet