jefffhaynes / BinarySerializer

A declarative serialization framework for controlling formatting of data at the byte and bit level using field bindings, converters, and code.
MIT License
290 stars 62 forks source link

No Exception Thrown When Deserialize Failed. #182

Closed luoxufeiyan closed 1 year ago

luoxufeiyan commented 2 years ago

Hi there,

similar to #159, BinarySerializer seems to swallow some exceptions under the deserialize procession, if data is not formed correctly. Is there any chance to throw this exception outside?

Here is my test data format:

public class TestFrame
{
    /// <summary>
    /// ListLength
    /// </summary>
    [FieldOrder(0)]
    public byte LstLen { get; set; }

    /// <summary>
    /// list
    /// </summary>
    [Description("list")]
    [FieldOrder(1)]
    [FieldEndianness(Endianness.Big)]
    [FieldCount(nameof(LstLen))]
    public List<byte> Lst { get; set; } = new List<byte> {1, 2, 3};

    /// <summary>
    /// Just an appendix
    /// </summary>
    [Description("appendix")]
    [FieldOrder(2)]
    public byte Appendix { get; set; } = 4;
}

The serialize process works properly, however, if I tried to change the ListLength property to mockup some testing, the deserializer did not throw an exception but set the result to null.

For instance, if I try to enlarge the ListLength property to occupy the last appendix byte, the deserializer did not throw exceptions, which could be hard for programmers to debug in real projects.

  var frame = new TestFrame();

  var serializer = new Serialization.BinarySerializer();
  using var stream = new MemoryStream();

  serializer.Serialize(stream, frame);
  //serializer.SerializeAsync(stream, frame);
  var bytes = stream.ToArray();
  // bytes now: 3, 1, 2, 3, 4,
  bytes[0] = 4;
  // bytes now: 4, 1, 2, 3, 4,
  var DesBak = serializer.Deserialize<TestFrame>(bytes);

Nonetheless, if I narrow the ListLength property, the deserializer works like a charm with the appendix just next to the next element after the list.

https://github.com/jefffhaynes/BinarySerializer/blob/c4e4222a6fb183bcd06bbe228763edcb96972a7d/BinarySerializer/Graph/ValueGraph/ObjectValueNode.cs#L136

I found that this issue may be related to the code above, is there any chance to throw this exception out?

Thanks for your time, merry Xmas.

jefffhaynes commented 2 years ago

I remember grappling with this a lot when I wrote that part. The problem is there are cases where it's valuable to "keep going" even if an object can't be fully reconstituted, so in those cases I simply null out the object. However, I also realize this flies in the face of fail-fast. Let me look at your specific use case and maybe it makes sense to make this behavior configurable or something.

luoxufeiyan commented 2 years ago

Thanks for your update.

One scenario for the BinarySerializer is to handle the communication between an upper computer and a lower PLC-liked device. Devices in both ends implement a pre-defined data structure and serialize it as data frames for communication. BinarySerilizer has been implemented on the upper computer side.

However, PLC devices may send tens of data frames per second, and it is hard to "mock data" on the PLC side, which makes it hard to reproduce this deserialize issue and makes the exception valuable.

Is there any change to provide a parameter to choose whether throw or omit the exception?

Thanks for your time.

celophi commented 1 year ago

+1

I would also like to see this as a configuration option in some format. I am using this library to achieve inter operation with a proprietary network protocol that I have no control over. Knowing when deserialization fails due to a change is necessary in order to maintain compatibility.

I will comment that, while this library provides so many options to handle complicated scenarios that are extremely helpful, it's a shame that this single issue makes it a deal breaker, and I have to resort to either forking or maintaining my own custom serializer.

jefffhaynes commented 1 year ago

Ok, I think it's been a year so I should probably do something with this :). My inclination is to add a property to the serializer that allows you to configure things like this. Not sure how else to do it since adding arguments to the methods would be a breaking change in the case of the async methods where the cancellation token should come last and be optional. Let me know if anyone has any better ideas.

jefffhaynes commented 1 year ago

Should be fixed now but let me know if you see something else