customerio-python icon indicating copy to clipboard operation
customerio-python copied to clipboard

Extend the default JSON serializer to fully support datetime instances

Open svvitale opened this issue 7 years ago • 7 comments

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.

svvitale avatar Oct 23 '18 21:10 svvitale

Also fixes #32 .

svvitale avatar Oct 24 '18 00:10 svvitale

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

svvitale avatar Jan 01 '19 18:01 svvitale

@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.

katewhite avatar Jan 03 '19 20:01 katewhite

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...)

svvitale avatar Jan 03 '19 21:01 svvitale

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}

jatinn avatar Jan 08 '19 15:01 jatinn

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.

svvitale avatar Jun 12 '19 16:06 svvitale

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

id0Sch avatar Sep 04 '19 12:09 id0Sch