Instead of using RepeatedField<> use List<> for c#

[mokoker]

Hi there,
I have been using protobuf with grpc in c# .net core 3.0 and generating collections as repeatedfield is causing lots of trouble for me, swagger is not generating json because they don't have setters and model binders can't bind to repeated fields. I don't want to create new dto objects, and had trouble creating model binders. Is no one passing these protobuf generated collections to web api controllers.

If I update to .net core 3.1 model binders bind to RepeatedField if its not readonly. Lack of setter is causing the problems with swagger and model binders.

Anyone knows the reason why there are no setters?

Thanks.

[mokoker]

public sealed partial class Test
{
    public const int ChildrenFieldNumber = 2;
    private static readonly FieldCodec<int> _repeated_children_codec
        = FieldCodec.ForInt32(18);
    private RepeatedField<int> children_ = new RepeatedField<int>();
    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
    public RepeatedField<int> Children
    {
        get { return children_; }
        set { children_ = value; }
    }
}

if i put the code above in the partial class everything works flawlessly, what is the problem with readonly and setter.

[bamancio-futura]

These fields with no setter make trouble working with GraphQL dotnet.

[elharo]

We (Google) used to have mutable protos. Experience taught us this was a very painful mistake. Immutability is a deliberate feature.

[erdalsivri]

We (Google) used to have mutable protos. Experience taught us this was a very painful mistake. Immutability is a deliberate feature.

It is still possible to mutate a RepeatedField with Add and AddRange methods similar to how it is possible to mutate other fields such as primitives or sub-messages. The only thing the current design ensures is that the reference to the list is immutable. Maybe Java uses builders and keeps the proto class immutable but in C# a proto class is definitely not immutable.

SomeProto proto = new SomeProto();
// Mutate some fields
user.Id = 4;
user.Name = "some name";

I am curious why the following doesn't work or what issues it might cause:

private List<string> _names = new List<string>();
public List<string> Names {
  get => _names;
  set {
    if (value == null) {
      throw new ArgumentNullException();
    }
    _names = value;
  }
}

Not having a setter is causing issues with EFCore. We cannot project database results into our proto DTO objects with repeated fields (there is an open FR on the EFCore side btw):

For example, the following doesn't work because Roles is RepeatedField without a setter:

_users.Select(u => new UserProto {
  Roles = { new RoleProto { ... } },
});

[elharo]

OK, this is C# which I think does use mutable protos for better or worse. I think the issue may be you need to use addFoo rather than setFooList or something like that. Reassigning to @jskeet for triage.

[jskeet]

Okay, there are two different requests here:

• The use of List<T> instead of RepeatedField<T>
• The use of a read-only property instead of having a setter as well

The TL;DR of the above is that I don't think we should change either decision, certainly not in the short term, as both would be breaking changes. We can certainly consider changing them in a future version, although I'd be reluctant to do so, personally.

The use of RepeatedField<T>

By using our own collection type instead of List<T>, we are able to put our own validation other other logic in the type. In particular:

• RepeatedField<T> does not permit any null elements
• RepeatedField<T> overrides GetHashCode and Equals in specific ways, using specific element equality comparisons

Using just plain List<T> would defer all validation to the point of serialization, which I'd prefer not to do - and require the equality/hash checks to be in extension/utility methods - along with all the serialization code which is currently part of RepeatedField<T>.

(All of this is also true for MapField<T> as well, of course.)

The property being read-only

I'll confess I don't remember all the reasons I made the property read-only. I do remember that an early prototype that allowed it to be mutable caused problems in all kinds of places... it's possible that some of that may have been related to the later-removed "freezable" feature.

One potential problem with this would be if an instance ever contained any information that had to stay the same beyond just the element type. For example, if in very rare cases we allowed null entries - e.g. a repeated field of a wrapper type. (I believe this was true at one point, but then removed.) With the property being read-only, everything can be certain that the collection itself stays the same, rather than being replaced by a "similar but not quite the same" instance. (The setter could potentially validate this additional state, but that in itself could create some surprising situations, I suspect.)

Compatibility

Changing the generated code to use List<T> or even an interface would be a breaking change in a fairly obvious way. Code that currently compiles such as:

RepeatedField<string> = message.SomeRepeatedStringField;

... would be broken by such a change.

Likewise currently code can store a reference to a repeated field from a message object, knowing that any change they make to the collection will still be related to that message object (and only that message object - no other messages will share the same collection). Adding a setter would violate that guarantee, so I'd consider it a breaking change.

Using protos with JSON

The challenges of using Swagger and GraphQL aren't entirely clear to me, but what I would say is that if you use any non-protobuf JSON serializer/parser with protobuf messages, things can easily go wrong. The protobuf JSON code knows exactly what to expect, in terms of the mapping between JSON property names and the C# properties, as well as well-known types such as Timestamp and Duration.

If you need to integrate with a different JSON framework, I'd strongly recommend building a converter that you can register that basically says "When you need to deserialize a protobuf message, use the protobuf code to do it." It may be somewhat inefficient to do that at the moment in places, but we could talk about that as a very separate issue. But changing the representation of collections to "somewhat" move the needle on other JSON frameworks parsing protobuf JSON as protobuf message objects feels like it's just going to lead to further disappointment as soon as you reach something like Timestamp.

[erdalsivri]

Even though I used List in the example, I think RepeatedField is fine. It implements IList so integrates well with other code. Sorry for the confusion.

However, not having a setter does cause some issues in EFCore projections. I do understand that there might be some internal assumptions that might be broken with a setter so feel free to close this bug. I hope EFCore team supports MemberMemberBinding soon: dotnet/efcore#16867. In the meantime, we are projecting first to an anonymous type and then to proto (outside of the LINQ expression) as a workaround:

var users = dbContext.Users
  .Select(u => new {
    Roles = u.Roles.Select(r => new UserRoleProto {
      // ...
    }).ToList(),
  })
  .AsEnumerable()
  .Select(u => new UserProto {
    Roles = { u.Roles },
  })
  .ToList();

[jskeet]

@erdalsivri: Right, understood. I'll close this as I can't see us changing it - at least not until we're making other major changes - but it is good to hear about the impact of various decisions.

[mirhagk]

Is the option of making other major changes on the table? In particular #9352 (inconsistencies between optional and wrapper types). Between this and that there's some major inconveniences with working with the library, essentially disallowing expressions for constructing protos.

That could be what is mentioned with GraphQL above, I'm not as familiar with that, but I've definitely come across the issue with many things that expect to be able to use expressions to map one type to another. EF being the most inconvenient example. This issue and the other turn a simple foo.Where(someCondition).Select(x=>new FooProto{...}) into a much more awkward version that has to create intermediate objects simply because protobufs can't safely assign a value to a field.

[jskeet]

Is the option of making other major changes on the table?

Potentially, but with a very high bar, basically.

[mirhagk]

May be worth collecting these with a tag or something rather than closing them outright. Each one individually may not warrant a breaking major version, but enough of them together could.