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

FieldFile is not JSON serializable Error & custom object serialization #35

Open
Nosudrum opened this issue Jul 27, 2024 · 14 comments
Open

Comments

@Nosudrum
Copy link

Hi there,

I have just installed this lib to try it out on a fairly large & complex Django project, and have encountered the following error when saving an object with a FileField

Object of type FieldFile is not JSON serializable

This leads me to the following questions:

  • This field type does not seem supported, what is the way forward ?
  • Is there a way to customize the serializer used in the webhook on a per-model basis ? Ideally I'd like to point towards the retrieve serializers used on the endpoints.
  • Is it possible to ensure that a webhook is only triggered when a field has changed, and not just on save ?

Any help is greatly appreciated :)

@danihodovic
Copy link
Owner

Good feedback. Some of this requires changes in django-webhook.

This field type does not seem supported, what is the way forward ?

You could change the encoder class to a custom JSON encoder. You could either return a string value for the file field or omit the field entirely. See: https://github.com/danihodovic/django-webhook/blob/master/django_webhook/settings.py#L4

# settings.py
DJANGO_WEBHOOK = {"PAYLOAD_ENCODER_CLASS": YourCustomEncoder}

Is there a way to customize the serializer used in the webhook on a per-model basis ? Ideally I'd like to point towards the retrieve serializers used on the endpoints.

It would require a change in django-webhook.

Is it possible to ensure that a webhook is only triggered when a field has changed, and not just on save ?

I think it would require a change in django-webhook in the sense of adding a custom filter function which determines if the webhook should be triggered. The filter function would need the pre and post model field values.

Let me know if you're interested in opening a pull-request.

@Nosudrum
Copy link
Author

Nosudrum commented Jul 28, 2024

Thanks for your answer @danihodovic. I think for us the way forward that makes the most sense is the second idea, i.e. the ability to specify a django serializer for each model to use in the webhook.
Would you be able to provide some guidance on how this could be implemented ?

Edit: another subsidiary question: how would we go about adding other fields to the webhook model ? e.g. name and contact or info fields to keep track of who is responsible for each webhook.

@danihodovic
Copy link
Owner

I'll get back to you later this week or the next

@Nosudrum
Copy link
Author

Nosudrum commented Aug 8, 2024

@danihodovic small reminder just in case :)

@danihodovic
Copy link
Owner

I'm having a busy few weeks, will post back soon

@danihodovic
Copy link
Owner

Is it possible to resolve 1) and 2) by providing a custom JSON encoder?

# settings.py
from foo.bar.baz import MyCustomEncoder
DJANGO_WEBHOOK = dict(PAYLOAD_ENCODER_CLASS=MyCustomEncoder)
# foo/bar/baz.py
from django.core.serializers.json import DjangoJSONEncoder
class MyCustomEncoder(DjangoJSONEncoder):
    def default(self, o):
         if isinstance(o, FileField):
         #...

@Nosudrum
Copy link
Author

Is it possible to resolve 1) and 2) by providing a custom JSON encoder?

I can't seem to make django-webhook use a custom encoder class, for some reason.
I have tried your option, and then even just tried raising an error whenever MyCustomEncoder.default() is called, and it does not seem to ever be.

class MyCustomEncoder(DjangoJSONEncoder):
    def default(self, o):
        raise ValueError("this should always be triggered")
        # if isinstance(o, FieldFile):
        #     raise TypeError("TEST FieldFile is not JSON serializable")
        #     return o.url
        # return super().default(o)


DJANGO_WEBHOOK = dict(
    MODELS=["api.Pad"],
    PAYLOAD_ENCODER_CLASS=MyCustomEncoder,
)

I still got the same kombu.exceptions.EncodeError: Object of type FieldFile is not JSON serializable error in both cases.

Is there perhaps another step I'm missing to enable the custom JSON encoder ?

@danihodovic
Copy link
Owner

Can you give 0.0.15 a try?

@Nosudrum
Copy link
Author

Can you give 0.0.15 a try?

The custom serializer is now indeed taken into account, but I am encountering celery/amqp [WinError 10061] issues with the exact setup documented here. I will try to debug it on my end and get back to you in a few days if I'm still stuck or if I have further questions.

@Nosudrum
Copy link
Author

@danihodovic Good news, I have figured things out on my end.
TL;DR I only had a look at the celery first steps with django guide, but hadn't seen the celery first steps guide, so I did not have RabbitMQ running (nor did I know what it was 🤦‍♂️ ).

A further Celery issue (ValueError: not enough values to unpack (expected 3, got 0)) was fixed by installing eventlet with pip and using celery -A LaunchLibrary worker -l INFO -P eventlet.

I now have a webhook working, as you can see in this example result received by webhook.site.

{
  "object": {
    "is_dirty": true,
    "id": 67,
    "launch_library_id": 176,
    "active": true,
    "name": "Ariane Launch Area 4",
    "string_name": "Ariane Launch Area 4 | Guiana Space Centre, French Guiana",
    "description": "ELA-4, is a launch pad and associated facilities at the Centre Spatial Guyanais in French Guiana. The complex is composed of a launch pad with mobile gantry, an horizontal assembly building and a dedicated launch operations building. ELA-4 is operated by Arianespace as part of the Ariane 6 program.",
    "info_url": null,
    "wiki_url": "https://en.wikipedia.org/wiki/Guiana_Space_Centre",
    "image": 1978,
    "map_url": "https://www.google.com/maps?q=5.256319,-52.786838",
    "country": 50,
    "latitude": 5.256319,
    "longitude": -52.786838,
    "location": 13,
    "map_refresh": false,
    "map_scale": 12,
    "map_image": "https://thespacedevs-prod.nyc3.digitaloceanspaces.com/media/map_images/pad_67_20200803143559.jpg",
    "orbital_launch_attempt_count": 1,
    "total_launch_count": 1,
    "fastest_turnaround": null
  },
  "topic": "api.Pad/update",
  "object_type": "api.Pad",
  "webhook_uuid": "dcdea97e-b2c3-43b8-b00f-b5528832c02a"
}

Circling back to my two questions above:

  • As you can see in this example the foreign keys are just IDs, instead of their own data (which we'd like), and all fields are sent (which we don't want, some are internal only).
    Any guidance on how it would be possible to use for the webhooks the same serializers we currently have on the retrieve endpoints ?
  • And yeah having other fields (e.g. name and contact or info) on the webhook model would be nice to keep track of who (third party users in our case) is responsible for each webhook.

@danihodovic
Copy link
Owner

Nice to see that you managed to get it working.

Maybe we should add some sort of middleware or filter function that allows you to modify the payload before it's sent. What do you think?

As a side note I'm curious what you're using django-webhook for. I think you're the first user posting about it.

@Nosudrum
Copy link
Author

Nosudrum commented Aug 21, 2024

Maybe we should add some sort of middleware or filter function that allows you to modify the payload before it's sent. What do you think?

I suppose so, yes. I don't know if it's possible to automatically figure out the endpoint serializer, but providing a mapping for each model would be acceptable. Ideally even a set of mappings of which one is chosen in each webhook object to provide some flexibility.
(e.g. so that we never break anything dependent on a webhook by changing the serializers in a new API version)

As a side note I'm curious what you're using django-webhook for. I think you're the first user posting about it.

The goal is to set it up for the Launch Library 2 API, where rapid data updates are important to our users (e.g. during rocket launches when it's a matter of seconds) but actual data changes in the db are sparse (sometimes only a handful daily). We hope to replace the most common API calls that reach our servers multiple times per second with just a few webhook calls to our biggest users.

Edit: another idea that would be really useful for us is the ability to customize on a webhook-per-webhook basis which model changes should trigger a webhook event. For example some of our users might only care about one or two models, while some would want all of them to have a perfect mirror of our db.

@danihodovic
Copy link
Owner

danihodovic commented Aug 23, 2024

Here's what I have in mind:

  1. Add a settings option for a transform function, for example "PAYLOAD_TRANSFORM" here. 2. The value of the setting should be a function which takes the payload as the argument. It should either return the payload (modified in your case) or None. If None is returned, the webhook shouldn't trigger. The default value of the function should be an identity function -- returning the argument unchanged.
  2. You can call the function in signals.py. If it returns None, don't send the webhook.
  3. Use import_string to dynamically import the function from a string path.
  4. Add a test similar to how the encoder is tested.
  5. Open a PR for review.
  6. Install the forked project via git to your larger project to test it out in action.
  7. If the tests pass and it works for you (step 7), we merge it.

Sample input to the function:

            payload_dict = dict(
                object=model_dict(instance),
                topic=topic,
                object_type=self.model_label,
                webhook_uuid=str(uuid),
            )

To understand how I test the sent webhook, see https://github.com/getsentry/responses.

What do you think?

@Nosudrum
Copy link
Author

Nosudrum commented Sep 5, 2024

FYI I have not forgotten this, on the contrary. I just need to focus on higher priority stuff until late october. Will get back to you then 🙂

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

2 participants