Skip to content
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.

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

[FEATURE REQUEST] SerializeWhile attribute, the negation of SerializeUntil #179

Open
sn4k3 opened this issue Dec 4, 2021 · 19 comments
Open
Assignees

Comments

@sn4k3
Copy link

sn4k3 commented Dec 4, 2021

Sometimes i can have unknown padding lenght that i need to skip. I don't find a attribute to by pass this so my proposal:

[SerializeWhile((byte)0)] public List<byte> Padding { get; set; }
[FieldLength(12)] public string Marker {get;set;}

I know that can mess up with numbers and eat that bytes, but in my case i have a obligatory string after the padding, so in this case is safe for the use :)

Example highlight in blue:
010Editor_2021-12-04_18-07-05

That padding length vary on files from 8 to 12 bytes

@brogdonm
Copy link

I would second a similar idea. I was looking at RFC8010 section 3.1.3 to describe the "additional attribute" (which is defined when a field is 0) and having a difficult time without this type of operator. I was thinking more of:

[ItemSerializeWhile(nameof(AttributeValue.NameLength), 0x00)]
public List<AttributeValue>? AdditionalAttributes { get; set; }

@jefffhaynes
Copy link
Owner

Working on this now. My plan is to add a configurable comparison operator to the SerializeUntil and ItemSerializeUntil attributes.

@sn4k3
Copy link
Author

sn4k3 commented Dec 23, 2021

Working on this now. My plan is to add a configurable comparison operator to the SerializeUntil and ItemSerializeUntil attributes.

Perfect :)

@jefffhaynes
Copy link
Owner

I've run into a tricky edge condition with this. The current behavior during serialization is to correctly set the termination value of the final item in a collection. This causes inconsistent behavior during serialization now that the termination condition is configurable. For example, if the condition is != then to what does the final value get set? It seems to me there are a number of options:

  1. Do nothing. This is nice because it's predictable but it can easily lead to unstable outputs and it is quite convenient having the framework set the final value of something if you're serializing a list and don't want to be bothered with setting the final value.
  2. Set it if the comparison operator is equal, otherwise do nothing.
  3. Set it if the comparison operator is equal, otherwise check the final value and throw if invalid.
  4. Set it if the comparison operator is equal, for everything else provide a default terminal value property on the attribute.
  5. Something else.

Not sure which I like the best yet. Possibly option 3 but when I run into complex cases like this it makes me feel like I'm missing something.

Obviously none of this is a problem during deserialization, which is what I assume your use cases are restricted to anyway :)

@jefffhaynes
Copy link
Owner

There's another confusing bit here where currently SerializeUntil means semantically, I want everything up to this point and then you can discard what comes next. If that changes to, I want to skip past everything and then get to the good stuff, it could be confusing to have to reconfigure SerializeUntil. ItemSerializeUntil has a LastItemMode and it might make sense to replicate that in SerializeUntil. However, that would mean in order to skip padding you would have to write:

[SerializeUntil(0, ComparisonOperator = ComparisonOperator.NotEqual, LastItemMode = LastItemMode.Defer]
public byte[] Padding { get; set; }

I guess that would work but it's kind of crappy.

@jefffhaynes
Copy link
Owner

The more I think about it, the more I like your suggestion @brogdonm. Maybe SerializeWhile and ItemSerializeWhile are more explicit. It would be nice to get ride of the last item mode altogether as well but I need to think about that.

@sn4k3
Copy link
Author

sn4k3 commented Dec 23, 2021

[SerializeUntil(0, ComparisonOperator = ComparisonOperator.NotEqual, LastItemMode = LastItemMode.Defer] - I think this is a bit confusing we are reverting logic.

But a ComparisonOperator makes sense on SerializeWhile, defaulting to Equal

@brogdonm
Copy link

@jefffhaynes Yeah, I forked your repo and tried a couple different routes and was getting stuck too, but I think most of it was me still learning the code.

In my case I have a deserialize and a serialize scenario.

@jefffhaynes
Copy link
Owner

Yeah @brogdonm I would say in some sense your scenario is more straightforward.

@jefffhaynes
Copy link
Owner

@sn4k3 I agree it's confusing. Unfortunately the current default behavior for SerializeUntil is to discard the terminating value. So in your example, the resulting string would be REVIEW rather than PREVIEW.

@sn4k3
Copy link
Author

sn4k3 commented Dec 24, 2021

@sn4k3 I agree it's confusing. Unfortunately the current default behavior for SerializeUntil is to discard the terminating value. So in your example, the resulting string would be REVIEW rather than PREVIEW.

Yup, i guessed that. We should be able to control whatever to discard or seek back the sizeof value

@jefffhaynes
Copy link
Owner

Sorry, tbh I'm recovering from surgery and the pain meds are not helping my thought process. Let me regroup and take another run at this in a bit :)

@brogdonm
Copy link

Sorry, tbh I'm recovering from surgery and the pain meds are not helping my thought process. Let me regroup and take another run at this in a bit :)

Man, hope you recover without issues.

@jefffhaynes
Copy link
Owner

Well this is bonkers. After spending many hours on this I've resigned myself to the fact that the only way to implement it is to remove byte arrays as a "primitive" type from the serialization system. That's something that always bothered me but I originally did for performance reasons. However, I've since made performance improvements to other areas that hopefully will allow me to make the change. So bear with me. This turned out to be a doozy...

@jefffhaynes
Copy link
Owner

jefffhaynes commented Jan 1, 2022

Ok, I think I have it working but currently there is almost a 10x performance hit when dealing with arrays.

I'll probably publish an alpha package tomorrow and maybe you can take a look and see if it checks all the boxes before I spend a lot of time optimizing.

@jefffhaynes
Copy link
Owner

https://www.nuget.org/packages/BinarySerializer/8.6.3-alpha

Good luck. Try SerializeWhile and ItemSerializeUntil with new configurations. Let me know if you need help with it. There's a slight gotcha with SerializeWhile but I'll wait to tell you :)

@brogdonm
Copy link

brogdonm commented Jan 5, 2022

It is nearly there, I have a regression issue with using Subtype | SubtypeDefault now. I have classes with:

  public class StringValue : Value<string>
  {
    [FieldOrder(0)]
    [FieldLength(nameof(AttributeValue.ValueLength), RelativeSourceMode = RelativeSourceMode.FindAncestor, AncestorType = typeof(AttributeValue))]
    public new string? Data { get; set; }
  }
  public abstract class Value<T> : Value
  {
    [FieldOrder(0)]
    [FieldLength(nameof(AttributeValue.ValueLength), RelativeSourceMode = RelativeSourceMode.FindAncestor, AncestorType = typeof(AttributeValue))]
    public new T? Data { get; set; }
  }

and

  public class Value
  {
    [FieldOrder(0)]
    [FieldLength(nameof(AttributeValue.ValueLength), RelativeSourceMode = RelativeSourceMode.FindAncestor, AncestorType = typeof(AttributeValue))]
    public byte[]? Data { get; set; }
  }

And using like so for serialization purposes:

    [Subtype(nameof(Tag), ValueTags.IntegerValue, typeof(IntegerValue), BindingMode = BindingMode.OneWay)]
    [Subtype(nameof(Tag), ValueTags.NaturalLanguage, typeof(StringValue), BindingMode = BindingMode.OneWay)]
    [SubtypeDefault(typeof(Value))]
    public Value? Value { get; set; }

And on previous released version, this was serializing correctly. But now, it is always serializing the data into the default class property (e.g. as a byte[] type) instead of the new overloaded property.

This was just beta/test code, so don't plan to use it, but figured I would at least mention there is a regression issue in the changes.

@brogdonm
Copy link

brogdonm commented Jan 5, 2022

@jefffhaynes I looked for your changes in a branch to debug, but didn't see the relevant changes. If you need anything else form me, please let me know.

With that said, it did appear to obey the ItemSerializeUntil at least. :)

@brogdonm
Copy link

@jefffhaynes any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants