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: setting lengths and field defaults #12

Open
gabearro opened this issue Sep 15, 2019 · 8 comments
Open

feature request: setting lengths and field defaults #12

gabearro opened this issue Sep 15, 2019 · 8 comments

Comments

@gabearro
Copy link

Here are some enhancements that I feel would greatly improve the quality of life of those implementing certain protocols. It would be greatly beneficial if we were able to set the type used to prefix a string/seq natively without making a new type custom type. An example of how this could be implemented would be like this:

serializable:
  type
    CoolStrings* = object
      set: {endian: bigEndian, strLength: int16, seqLength:  int64}
      strings*: seq[string]

This would provide the user with a level of flexibility that is honestly so nice due to the simplicity of toggling said option.

Another feature which would be nice would be the ability to set the default value of a field, at least for me I'm re-implementing a certain games protocol and some packets might have additional bytes at the end which might get parsed into their respective variables. An example of this would be this:

serializable:
  type
    SpecialPacketWithFieldsThatMightExist* = object
      name*: string
      specialfield*: int32 as {default: 40}

This would give us the flexibility to give fields a default value other than 0 as is currently implemented in nim. This would allow us to parse packets which have the possibility of containing optional/additional data.

@xomachine
Copy link
Owner

xomachine commented Sep 15, 2019

Well, the couple of points in this proposal still look unclear to me.

  1. The behavior of the library when the string/seq size if bigger than limits of the type that set explicitly.
    It looks like the runtime error should be raised, but is it really worth to get runtime error where we can perform the compile-time check by providing a certain type? I think the prevention of the misbehavior by design is better than catching exceptions.

  2. Does the default value should work like a constant? I mean we should place provided value to the field regardless of the real value of the field at serialization stage and check if the field has provided value when deserializing, right? But what we should do if the value differs? Raising an exception could be quite expensive and the next thing someone may ask is the way to provide a callback that should be called on the value mismatch... Could you please provide more detailed description of what do you expect?

@gabearro
Copy link
Author

gabearro commented Sep 15, 2019

  1. I think you're right, after thinking about it, it seems like way more of a hassle to implement and not worth it because of the limited use cases and the easy solution of making a custom type. Its mostly a QoL if anything but it definitely doesn't need to be done.

  2. For this feature I was hoping for something that would unpack the data into a field only if the buffer's length was long enough for the type. An example of what I currently do in python is this https://github.com/Riderfighter/PyRelay/blob/master/Packets.py#L1152 . The behavior of "default" should check if the buffer is long enough for the type otherwise apply the default value. One issue I see is that this will definitely be an issue if someone sets a default in the beginning/middle of a struct but if there was a way to check that its one of the last fields and the buffer could/couldn't support the type's length it would definitely be plausible.

Here is a case where a compile time error should be raised because the default is not at the end of a struct:

serializable:
  type
    DefaultFail* = object
      bulletId*: int8
      ownerId*: int32 as {default: 40}
      bulletType*: int8

Here is what correct usage should look like because the defaults are the last fields:

serializable:
  type
    DefaultFail* = object
      bulletId*: int8
      ownerId*: int32 as {default: 40}
      bulletType*: int8 as {default: 87}

I Hope that this makes sense regarding my second feature request, its been itching me to have something like this in NESM, I think that the usability would outweigh how expensive the feature itself would be.

@gabearro
Copy link
Author

gabearro commented Sep 27, 2019

Another QoL thing that I thought about and would love to have implemented would be setting a field to be the sizeof the current struct or a field that has a struct. I believe that this would definitely be possible due to the fact that NESM generates the procs that tell you the size of the current serializable struct.

Currently if you want to set a field to be the size of a field you must do something like this:

serializable:
    type
        customdata = object
            thing: int8
            thing2: int32
        serializablethingwdata = object
            length: int32
            data: customdata

var test: customdata
test.length = test.data.size

But just imagine how slick NESM would be if you could reduce those extra lines of defining an object's length to just {sizeof: {}.data} or the size of the whole object to just {sizeof: {}}

An example of how this might be used is this:

serializable:
    type
        customdata = object
            thing: int8
            thing2: int32
        serializablethingwdata = object
            length: int32 as {sizeof: {}.data}
            data: customdata
        serializablethingallsize = object
            length: int32 as {sizeof: {}}
            data: customdata

You don't have to implement this but this is a really cool thing that I'm sure others will truly enjoy.

This really makes me want to learn how to write macros, I feel bad requesting all this stuff without the ability to help :L

@xomachine
Copy link
Owner

Well, NESM was designed as a tool to [de]serialize structures known at compile time. I wonder that kind of non-periodic(like seq) structure requires its size to be placed in header. Which protocol are you trying to implement?

@gabearro
Copy link
Author

gabearro commented Sep 27, 2019

I'm currently trying to implement the bittorrent protocol for fun and its led me to think of QoL features such as the ones I outlined above. Originally I was remaking the protocol that www.realmofthemadgod.com uses for the game but I ended up being up unable to do that due to some packets having data that might have a custom value or a default value.

For bittorrent the header is the length, int32, of the packet which has the value of just the size of the packet's data while realmofthemadgod's header has a length, int32, which is the total size of the packet.

@FedericoCeratto
Copy link
Contributor

@xomachine I also initially thought that NESM was meant to be used for writing parsers for existing protocols (network and other).

xomachine added a commit that referenced this issue Oct 27, 2019
related to #12

Adds the deserialize proc overload that accepts mutable object to fill with deserialized
content. If the deserialization was not complete (due to lack of data in the stream)
the part of the object passed for which data has not obtained will be left untouched
@xomachine
Copy link
Owner

I implemented the default values but in slightly different way. I think it is better to pass a prebaked object with default values to the deserialize function instead of setting them in the type declaration. Try it in the devel branch!

@FedericoCeratto That is true, but I did not expect cases like this when started the developement of the library. So it is not quite easy to add the features related to fields autofilling without creating a mess in the source code (it is already quite complicated in my opinion).

@gabearro
Copy link
Author

@xomachine Thank you for this I will check it out!

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