-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
deficiency in new webhook implementation #2137
Comments
@ridoline I am unable to reproduce this with a Date custom field on the Device model. Please provide the exact steps necessary to raise this issue. |
@lawpwins, I suppose it might have been an issue with my data or has been resolved, as I recently refreshed my development box with production data and the issue is no longer manifesting itself. |
I played with this some last night and was able to raise the issue a handful of times, but I have yet to find a root cause or even a reliable way of triggering the issue. I will be digging into this more. |
This coerses all date type custom field values to strings. This is due to a problem with serializing datetime objects.
I finally ran this down. There are two separate issues in play. First, date type custom field values are currently actual datetime objects and the default json serializer in the requests library doesn't handle this well. I am still a little unclear why DRF is okay with this, since we are using the same API serializers. Second, the current signaling system for Webhooks suffers from the same problems of related objects (custom fields and tags) not being associated at the time of the post_save signal, as the original Change Logging implementation. This was addresses in Change Logging in df1f339. I think it makes a lot of sense to wrap the Webhook signaling into the ChangeLoggingMiddleware since the mechanics of both features are very similar. |
Just to be clear, this was broken out from #2282 and thus this issue still needs to be addressed. |
I finally nailed the root cause down. DRF processes the JSON encoding after the serialization, so by manually using the serialisers we miss the DRF specific encoding which makes use of there own JSON encoder class which correctly handles datatime objects (and many others) https://github.com/encode/django-rest-framework/blob/7b1582e00e91001ec07cd394520ec5cdd2c2add1/rest_framework/utils/encoders.py#L34. Currently we are using the built-in JSON encoder of the requests library, so we simply need to specify an alternate encoder to fix this. |
Force webhooks to use the same JSONEncoder class as DRF - fixes #2137
Issue type
[ ] Feature request
[X] Bug report
[ ] Documentation
Environment
Description
Testing out the webhook implementation and discovered the following bug:
when a model contains a custom field of type date the worker is unable to serialize the data for transmission
The text was updated successfully, but these errors were encountered: