msgpack / msgpack-cli

MessagePack implementation for Common Language Infrastructure / msgpack.org[C#]
http://msgpack.org
Apache License 2.0
832 stars 174 forks source link

ReadOnly and WriteOnly properties aren't excluded from serialization #21

Closed joaofernandes closed 10 years ago

joaofernandes commented 10 years ago

Trying to serialise/deserialise this class, messagePack fails because an exception is thrown because no setter is available.

I've noticed that while parsing members, it will take all properties or variables but doesn't filter out properties that are readonly or writeonly.

Class TestClass Public item As String = "" Public ReadOnly Property readonlyProperty As String Get Return item End Get End Property

Private _getSetproperty As String = ""
Public Property getSetPproperty() As String
    Get
        Return _getSetproperty
    End Get
    Set(ByVal value As String)
        _getSetproperty = value
    End Set
End Property

End Class

yfakariya commented 10 years ago

I think, in the .NET world, there is a principal that a serializer should not treat non public members unless user(developer) explicitly requires it. So MsgPack for CLI does not serialize non public members as you saw.

I guess that your use case should be covered that you mark your type with traditional [Serializable] attribute when you serialize non public type. How do you think?

yfakariya commented 10 years ago

I've changed my thoughts. When a type has non-publicly-accessible members, it could have complex implementations like hashtables. So, they are not able to be handled even if the serializer just handle non-public members. It looks better that msgpack for cli adds ISerializationFormatter implementations on FCL runtime serialization stack. Although it is very rich and powerful, it is limited for platforms and performance. So, I will add new serialization formatter class in future.

Thank you for your suggestion, and it is welcome to post additional comment :)

yfakariya commented 10 years ago

...and, you can opt out read-only and/or write-only members by marking members with some custom attributes. See https://github.com/msgpack/msgpack-cli/wiki/Serialization-attributes for details.

joaofernandes commented 10 years ago

Sorry for the delay,I'll try to explain properly my issue:

Our classes only use <XMLIgnore()> to ignore properties and readonly properties where ignored automatically by XMLSerializer/Deserializer but MessagePack doesn't. As I mentioned before, a readOnly property does reach the serialization process if none of these rules are applied to my object https://github.com/msgpack/msgpack-cli/wiki/Serialization-attributes which in my opinion they probably shouldn't unless they are marked to be included. Sorry if I'm not clear enough.

mvodep commented 10 years ago

Hmm How is it possible to serialize a readonly property either? public string Foo { get; private set; } Throws an exception that set is not possible

joaofernandes commented 10 years ago

That's my point, it should ignore that property but messagePack Isn't doing so. I don't have any kind of custom metadata as described in the previous link besides XMLIgnore() in some properties which isn't the case for this specific because it's readOnly and XMLSerializer/Deserializer already ignore read-only props.

mvodep commented 10 years ago

Hmm. Ok - i have the opposite case ;-) I would be happy if it would work in C# - it doesnt ;-( I had to make all my immutable objects public :-((

yfakariya commented 10 years ago

Sorry for delay. I think that I spoke too soon because your needs might be satisfied with existing features.

@joaofernandes

Although you cannot opt-out read only property as you see, you can do equivalent with opt-in manner like following:

Class TestClass
    Public item As String = ""
    Public ReadOnly Property readonlyProperty As String
        Get
            Return item
        End Get
    End Property
    ' ...
    ' Mark at least one read-write members with MessagePackMemberAttribute
    <MessagePackMember( 0 )>
    Public Property Foo As Integer
        ...
    End Property

Not that when you use MessagePackMemberAttribute, members marked with MessagePackMemberAttribute attribute are only serialized/deserialized. Non marked members are just ignored.

@mvodep

If you want to serialize your immutable class, consider implementing IPackable and/or IUnpackable to customize serialization/deserialization behavior. It similar to ISerializable of .NET runtime serialization.

joaofernandes commented 10 years ago

Thanks, I know that there is the new metadata which allow me to opt-in but unfortunately our current code base is already composed of hundreds of serializable classes and hand changing all properties to manually add the metadata is an issue. Thanks for your time.

mvodep commented 10 years ago

@yfakariya: Thx - i checked the Image example - lots of cross cutting code :-( Is there a helper function which can map the array to members with [MessagePackMember(X)]?

yfakariya commented 10 years ago

@mvodep

Unfortunately, there is not. But I guess you can do it as following:

  1. Define mutable objects for serialization. They have enough fields to serialize states for correspond immutables.
  2. Wrap mutable objects with your immutable type, or copy from/to immutables in IPackable/IUnpackable methods.
  3. Get the serializer instance for correspond mutable in IPackable/IUnpackable methods of immutables, then invoke its PackTo/UnpackFrom.

Following code is pseudo code of IPackable.PackTo:

public class MyImmutable : Ipavkable  IUnpackable
{
    private MyMutable state;

    public String TheProp1
    {
        get { return state.TheProp1; }
    }
    // rest of getters to acomplish immutable object here...

    void IPackable.PackTo(Packer packer, PackingOptions options)
    {
        // note: If you use copying pattern, instantiate mutable and copy state here.

        // Get serializer.
        var serializer = MessagePackSerializer.Create<MyMutable>();
        // Pack mutable instead of immutable.
        serializr.PackTo(packer, state);
    }

    // IUnpackable members here...
}
mvodep commented 10 years ago

I'll give it a try - thx :)

AaronTorgerson commented 10 years ago

This seems like a flaw in the design. For public properties, a public getter with no (or no public) setter should have its value serialized to the msgpack content, but should not have its value set during deserialization; likewise a public setter with no (or no public) getter should have its value set during deserialization, but should not be present in the serialized content. Readonly properties need to be permitted without special attributes, etc, to allow for computed values that derive their value from other read/write fields and properties. Many other serializers work this way and mskpack should, too.

yfakariya commented 10 years ago

@AaronTorgerson

I agree your opinion, and pr #32 and recent changes on 0.5 branch fix this design issue.