Skip to content

Custom Distinct DateTime type breaks serialization of object to string? #38

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

Closed
PhilippMDoerner opened this issue Jan 1, 2022 · 6 comments

Comments

@PhilippMDoerner
Copy link
Contributor

Heyho!
I'm using jsony in a web-application. There I want to use it to serialize an de-serialize JSON strings into objects, specifically of norm-objects (an ORM) which is filled with data coming from a database.
Due to the way norm handles Datetimes, I had to implement a custom DateTime type that borrows/reimplements DateTime's functionality, that appears to cause some problems.

What I found is that, when doing so, jsony drops part of the serialization string silently, without an error message.
Here is a minimum example that demonstrates the issue when I run it on "https://play.nim-lang.org/".

import jsony
import options
import times

type DjangoDateTime* = distinct DateTime
proc format*(x: DjangoDateTime, f: string, loc: DateTimeLocale = DefaultLocale): string =
    let trueDt = x.DateTime 
    result = trueDt.format(f, loc)
    
proc dumpHook*(s: var string, value: DjangoDateTime) =
    s = value.format("yyyy-MM-dd HH:mm:ss'.'ffffff")

proc now*(): DjangoDateTime = DjangoDateTime(times.now())


type 
    A = object
        nameA: string
        datetimeA: DjangoDateTime


var a = A(nameA: "The name of A", datetimeA: now())
echo a.toJson()

This should print out {"nameA": "The name of A", "datetimeA": "<FORMATTED CURRENT DATETIME>"}.
Instead it prints out <FORMATTED CURRENT DATETIME>}.
It drops the parts of the string that come before DjangoDateTime is converted into a string.

I am not quite sure what is happening here, though I can only assume that it's DjangoDateTime causing this. How, I'm not sure.

@PhilippMDoerner
Copy link
Contributor Author

PhilippMDoerner commented Jan 1, 2022

I'm an idiot, my bad.

To myself and others that potentially run into this issue:
DUMP HOOKS RECEIVE IN S THE FULL CURRENT JSON STRING, THAT IS NOT JUST A VARIABLE FOR YOUR CONVENIENCE TO DUMP YOUR SERIALIZED STRING INTO
I did not comprehend that, thus this line here replaced the entire previous Json String with just the serialized DateTime
s = value.format("yyyy-MM-dd HH:mm:ss'.'ffffff") is a terrible idea. It has to be s.add value.format("yyyy-MM-dd HH:mm:ss'.'ffffff")

@beef331
Copy link

beef331 commented Jan 1, 2022

To prevent this from happening in the future the mutable string could be changed into a var JsonyString which would be a distinct string and proc add(jstr: var JsonyString, s: string) which quotes s when adding it meaning any string operations would require manually converting to string on the input, making it less error prone and more typesafe.

@treeform
Copy link
Owner

treeform commented Jan 1, 2022

@beef331 what are your thoughts on #37 ?

@treeform
Copy link
Owner

treeform commented Jan 1, 2022

@PhilippMDoerner I am glad you figured it out.

The readme even has a section for datetime: https://github.com/treeform/jsony#proc-parsehook-can-be-used-to-do-anything

I have spent a lot of time dealing with time in web applications I have even written my own calendar + timezone library https://github.com/treeform/chrono .

I highly recommend using float64 seconds sense 1970 - the unix epoch to transfer times.

@PhilippMDoerner
Copy link
Contributor Author

Heyho @treeform
I was tempted to move to that because if I had, I could've saved myself around 50 lines of code etc.

The necessity to stick with another DateTime type lies within where my database comes from: I already have one and it was generated through Django's ORM (in general, I'm re-implementing an already existing backend of a sideproject of mine for learning purposes). Django sets datetime columns up so that they store datetime as a string with the pattern yyyy-MM-dd HH:mm:ss'.'ffffff. That's why I'm even bothering with all this.

I'll definitely keep moving to unix timestamps in mind, since it'd make things simpler, but I didn't want to include a database migration in this phase of the project.

@treeform
Copy link
Owner

treeform commented Jan 1, 2022

Your reasoning for sticking yyyy-MM-dd HH:mm:ss makes sense.

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

No branches or pull requests

3 participants