Override CborReader
Odonno opened this issue · 26 comments
I need to override methods in the CborReader class.
@mcatanzariti Do you have an idea how to achieve this?
My current approach would be:
- Mark methods of
CborReaderaspublic virtual - Pass down a custom
CborReadertoCborconstructor - Use custom
CborReaderor the default one if no custom is passed
@Odonno Could explain me what you want to achieve by overriding CborReader? We could find together the best solution for your requirements
I need to override at least ReadNull.
I also need to override ReadDecimal, ReadFloat and ReadDouble but maybe those can be replaced with Converters.
I am still in need for overriding ReadNull as this method is already used by a lot of methods.
To be more explicit, I have a Semantic Tag that corresponds to a null value. I have to override ReadNull otherwise my tests fails.
Instead of letting users override ReadNull because it is very deep and core, what do you think about letting users configure Semantic tags either in the global CborOptions or by property attributes?
About overriding ReadDecimal, ReadFloat and ReadDouble is it also related to semantic tags or do you need something else?
Thank you,
Michaël
About ReadNull you want only a specific semantic tag or you want a specific semantic tag+ the cbor primitive null (22)?
About
ReadNullyou want only a specific semantic tag or you want a specific semantic tag+ the cbor primitive null (22)?
The Semantic Tag would be sufficient. But we can do Semantic Tag + null primitive if that's easier this way.
About overriding
ReadDecimal,ReadFloatandReadDoubleis it also related to semantic tags or do you need something else?
Only ReadDecimal would have a semantic tag. I made my own converter for this so this would not be of any use overriding it.
For ReadSingle and ReadDouble, I am curious why string parsing if not implemented by default? I suppose this does not need override and be implemented in those methods right away.
Also something I noticed when writing custom converters. The semantic tag is no longer available when we are in the Read method. Is this by design? Is there a way to enforce a semantic for a particular converter or do you have to create an issue? Or maybe as simple as this, do we really need to enforce this? This is a question I am asking to myself because the object model should follow, right?
I could implement a better support of semantic tags but I need for derailed information about what you need exactly
To answer your question, as C# is a strongly type language, I rely C# type rather than semantic tags to know what I should deserialize, so I currently ignore semantic tags in CborReader. Depending on what you need, it could be improved
I made the change in #124 that solves the problem for float and double.
Still need the change for ReadNull. Do you a specific implementation in mind? As I only need this for null, I could only need to pass down Semantic tag mapping for null. Not really a generic solution that will work. Do we need something generic for this? I don't know.
Either a global option in CborOption to specify the expected tag for ReadNull.
Or a C# attribute on a specific nullable property of a class to specify the expected tag.
Both could be supported.
Do you need it to be global or specified as an attribute for specific properties of specific classes?
It should be global while still supporting the existing null value check.
@Odonno something is not clear about null semantic tag.
if you want me to read semantic tag + cbor null primitive (22), it should already work because the semantic tag skipped (without being checked) in CborReader.ReadNull but it is still parsed.
Consequently, what you need should be to just check the semantic tag without expecting the cbor null primitive, right?
Please, could you provide an example of a Cbor buffer with the exact pattern you want to be parsed by the CborReader struct?
Thank you,
Michaël
Actually orphelin semantic tags are not allowed by the CBOR specification. The tag must be followed by a single data item of any type.
Consequently, I did not catch what you require that is not already supported by the library...
To be more explicit, I have a Semantic Tag that corresponds to a null value. I have to override ReadNull otherwise my tests fails.
Could you paste here the failing test, please?
Yes, that was what I was thinking at first. But when I tested it, it doesn't work. I suppose this is because you're expecting a CborMajorType.Primitive or something and not skipping the semantic tag because you're gonna need later. That way you're checking for null on the semantic tag instead of the primitive.
Here is a sample code that fixes my issue:
public bool ReadNull()
{
return ReadNone() || Accept(CborPrimitive.Null);
}
public bool ReadNone()
{
if (TryReadSemanticTag(out var semanticTag) && semanticTag == 6)
{
_state = CborReaderState.Data;
return true;
}
return false;
}It will work with a C6F6 hexa binary.
Of course I don't want to make a PR that integrates this code because it is specific to my project. Just need to find a proper solution.
Could work to have something like that:
private readonly ulong[]? _allowedSemanticTagsForNull;
public bool ReadNull()
{
return AcceptSemanticTag(_allowedSemanticTagsForNull) || Accept(CborPrimitive.Null);
}
// ...Yes. ReadNull is working fine in this context. I am sorry I made a too simple example.
The bug happens when you are trying to read something else with ReadNull being checked first. Like Read from ObjectConverter or maybe a simple ReadString I suppose. I will try to write an example when I can.
Please provide a full test to reproduce the problem
I just added 3 successful tests:
- 1 with ReadString
- 1 with a class with null string property
- 1 with a class with null class property
Ok. I missed the SkipSemanticTag in ReadNull.
I do not know why I didn't have it. You can close your PR. Sorry for losing time on this matter.
I suppose all my problems are gone now. Do you have a release window with latest features?
Hi @Odonno,
No problem. It is pleasure to have tech conversations with passionated and caring people. It's why I'm investing on open source projects. And just by using this library, you make my day.
I'm going to deploy a new version today.
À bientôt,
Michaël
Same. Thank you for your effort to make this package viable. I never said it but it is really well designed. Hoping that I do not miss any particular use case but I have the feeling it should be the standard into what a library should look like, for a different serialization format for example.
Merci beaucoup pour ta réactivité.
A new version is published on nuget: https://github.com/dahomey-technologies/Dahomey.Cbor/releases/tag/1.23.0