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

Extend the default JSON serializer to fully support datetime instances #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

svvitale
Copy link

The existing _sanitize() method only cleaned datetime instances if they were root values in the request payload. By extending the default JSONEncoder class, we can easily support serialization of datetime values anywhere in the request payload structure (even several levels deep). This is particularly useful for calls to track(), where all extra event data is stored one level deeper in the payload.

This also adds a framework for easily supporting serialization of additional complex data types.

…ll levels.

The existing _sanitize() method only cleaned datetime instances if they were root values in the request payload.  By extending the default JSONEncoder class, we can easily support serialization of datetime values anywhere in the request payload structure (even several levels deep).  This is particularly useful for calls to track(), where all extra event data is stored one level deeper in the payload.
@svvitale
Copy link
Author

Also fixes #32 .

@svvitale
Copy link
Author

svvitale commented Jan 1, 2019

Any chance I can get this reviewed? @katewhite can you help?

@katewhite
Copy link

@svvitale - Sorry for the lag here! One of our backend engineers (our Python guru) will be reviewing this next week when he's back from vacation.

@svvitale
Copy link
Author

svvitale commented Jan 3, 2019

Thank you! (I have some additional changes that add support for some of the beta API, but I thought I'd get this wrapped up first...)

@jatinn
Copy link
Contributor

jatinn commented Jan 8, 2019

Thanks for this PR @svvitale. I was trying this out and noticed that nan values were not being encoded as null for me. Here is a simplified script that I tried, could you try this out and see if it returns null and let me know what version of python you are using. Thanks.

Also, its great to hear that you find value in our beta-api however we have intentionally not updated our client libraries to add support for it at this time.

import json

class CustomJSONEncoder(json.JSONEncoder):
    def default(self, obj):
        if isinstance(obj, float) and math.isnan(obj):
            return None
        return json.JSONEncoder.default(self, obj)

print(json.dumps({'nan': float('nan')}, cls=CustomJSONEncoder))
~/Desktop (py37)
⧰ python -V
Python 3.7.2

~/Desktop (py37)
⧰ python encode_test.py
{"nan": NaN}

~/Desktop (py27)
⧰ python -V
Python 2.7.15 :: Anaconda, Inc.

~/Desktop (py27)
⧰ python encode_test.py
{"nan": NaN}

@svvitale
Copy link
Author

Hi @jatinn! Sorry for my slow reply here. I would actually suggest a change in behavior around serializing NaN, Infinity, and -Infinity values. Python's JSON encoder unfortunately doesn't allow for overriding the encoding of built-in types (see this bug report: https://bugs.python.org/issue30343). This is why my custom encoder appears to not be working for NaN values.

From the NaN handling in the customer.io code, it seems that you'd like to avoid sending NaN values to the backend. My suggestion is to use the allow_nan=False option when calling the Python encoder. This will result in raising a ValueError exception if a NaN value is found. This changes the behavior for users of this library, but ultimately it's pointing out an inconsistency in the client's code.

I'm not sure if this will be acceptable to you and the product management team, but I think it's a good direction long term in that it allows for further customization of the JSON encoder at all levels.

If this is acceptable, I have the changes queued up, and will gladly add them (along with unit tests) to this PR.

@id0Sch
Copy link

id0Sch commented Sep 4, 2019

any update on this issue?
we identify datetime attributes and want to have them in an event, but it doesn't work ;(

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

Successfully merging this pull request may close these issues.

4 participants