Skip to content

Improve read-only collection/dictionary support #822

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve read-only collection/dictionary support #822

wants to merge 1 commit into from

Conversation

AlexMihalev
Copy link

I steel think that ability to support data model with read-only properties can be very valuable especially keeping in mind the latest C# features like records and nullability checks, for example:

#nullable enable
[ProtoContract]
public record SomeClass
{
    // is better than following variants because 
    [ProtoMember(1)]
    public IReadOnlyCollection<string> Values1 { get; init; } = Array.Empty<string>();

    // every time during reading we should check is Values2 null, 
    // and after that is empty or not (which is effectively covered by foreach loop or linq operations)
    [ProtoMember(2)]
    public IReadOnlyCollection<string>? Values2 { get; init; }

    // in this case, we lose immutability because the collection has official writable interface
    ICollection<string>? _values3;
    [ProtoMember(3)]
    public ICollection<string> Values3 => _values3 ??= new List<string>();

    // this case is already supported but require in most cases repacking because Immutable interfaces
    // are not supported by mutable collection (which are still used in most cases) 
    // in contradiction to ReadOnly interfaces
    [ProtoMember(4)]
    public IImmutableList<string> Values4 { get; init; } = ImmutableList.Create<string>();
}

And I understand that the proposed solution will effectively break backward compatibility in some, I think, questionable cases (you can see deleted test), so I will not be upset at all if you reject this pull request.

@mgravell
Copy link
Member

mgravell commented Aug 8, 2021

A couple of observations:

  1. removing ProtobufNet_SupportsNakedEnumerables_ButMustBeAddable() seems undesirable; why is that being removed? If it is now failing, then that means there is a breaking change
  2. I'm ... dubious about exploring the fact that types turn out to be mutable; they've given us the immutable API surface, and IMO we should respect that, even if it means we need to create a clone of the collection that we then mutate

@mgravell
Copy link
Member

mgravell commented Aug 8, 2021

(of course, if that's what we already do for lists, then it becomes more doable/consistent; is that the driver here?)

@AlexMihalev
Copy link
Author

A couple of observations:

  1. removing ProtobufNet_SupportsNakedEnumerables_ButMustBeAddable() seems undesirable; why is that being removed? If it is now failing, then that means there is a breaking change

Yep, I remove it because it's started failing, actually, it stops throwing an exception, and yes it looks like a breaking change but, existing behavior looks strange:

  • property defined as an immutable collection with immutable default value - works like
  • property defined as a read-only collection (including IEnumerable)
    • with read-only default value - throw an exception
    • with writeable default value - works fine
  1. I'm ... dubious about exploring the fact that types turn out to be mutable; they've given us the immutable API surface, and IMO we should respect that, even if it means we need to create a clone of the collection that we then mutate

If I understand correctly, the same behavior already implemented in EnumerableSerializer even property is defined as IEnumerable (which can't be writable by definition) require ICollection implementation which is used to bypath actual property type. And deleted test just demonstrate this behavior.

Actually, all I want is just to have the ability to define an immutable interface without nullable collections, because null and empty collections are the same especially in the case of protobuf encoding, but with a second much easy to live.

@AlexMihalev
Copy link
Author

For some reason, I can guess some functionality around merging buffers? The current implementation tries to fill provided default property value even its default value is a read-only collection ignoring property setter existence. And you are right best solution will be to recreate the whole collection as it's done in ImmutableXXX implementations but in this case, we will have even more breaking changes (deleted test will definitely fail).

@mgravell
Copy link
Member

mgravell commented Aug 8, 2021 via email

@AlexMihalev
Copy link
Author

Any update on this?

@AlexMihalev AlexMihalev mentioned this pull request Oct 13, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AlexMihalev @mgravell