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

[Improve Docs] Should include documentation for setting custom Observer serializer #59

Open
HtheChemist opened this issue Feb 24, 2021 · 7 comments

Comments

@HtheChemist
Copy link

Describe the bug
if the Observer._serializer is not empty, the .serialize function will try to use it to generate the message_body. Using a standard ModelSerializer from DRF will cause a TypeError because of the arguments.

Removing self from the call will then create another TypeError unexpected keyword because the Field init does not support kwargs.

This part of the code is not covered in the test.

To Reproduce
Steps to reproduce the behavior:

  1. Create a custom Observer extending ModelObserver (or similar).
  2. Override the init method, while first calling the super() then assigning a Serializer using self.serializer(serializer)
  3. Subscribe to the consumer using the custom Observer

Expected behavior
The serializer should be able to serialize the instance.

LOG
No Log

Additional context
Simply using self._serializer(instance).data will return the serialized instance under a ReturnDict format that can be converted into JSON.

Since this options is not covered in the test, I would guess that it is a very specific edge case or that it is a placeholder for something else but the fix seems to be simple.

@hishnash
Copy link
Member

This should be better documented, the _serializer should be a function not an instance of a DRF ModelSerializer.

The main reason is you likely need to include information such as the signal since the model observer will fire on all sorts of events include deleting a model.

Could consider detecting if it is an instance of ModelSerializer and calling it differently.

Im assuming your reason for changing Serialization on the observer is to reduce database load if you have many subscribers?

@HtheChemist
Copy link
Author

Thank you. I could not make sense from how the _serializer was set by the code and the tests, hence why I assumed it would use a standard ModelSerializer class.

I am generating dynamic observer with a single common consumer through a configuration file, having the message serialized directly in the Observer allows me to have a single end point for all of them that wrap around my standard DRF view directly.

A little like that:

WEBSOCKET_OBSERVER = {
    'observable_models': {
        'notifications': {
            'model': 'app.notifications.models.Notification',
            'function': 'general_observer',
            'route': '/api/notifications/',
            'serializer': 'app.notifications.serializers.NotificationSerializer'
        }
    }
}

@hishnash
Copy link
Member

oh ok, for this in the bast i have used a multiplexer so that from the client side it is one connection but on the server side i have a consumer for each type, i found this gives me more flexibility when it comes to things like permission checks etc.

@HtheChemist
Copy link
Author

This is the general idea. The generic consumer act as a multiplexer and redirect the signal to the right function and check the permission through the DRF Api route. This way I don't have to write a consumer for each model and can simply "activate" the consumer in the setting.

@hishnash
Copy link
Member

hishnash commented Feb 25, 2021

It is with noting that the serializer attached to the model observer is called on the thread were the object is changed. Then the result is sent over the channel layer to all the subscribe consumer instances. This means you do not have access to the user during serialization so if you have any data that you want to hide from some users etc that is not possible.

This is why the default behaviour of the model observer is to just send the event and object id. then on the consumer side that object id can be used to retrieve the instance and we can do user level permission checks and custom serialization however this does come at a cost as each subscribed consumer will need to do a DB query to retrieve the data for serialization.

@HtheChemist
Copy link
Author

Yes this was noted. Another permission check need to be done on the consumer side to ensure that the user has the permission to see the signal event.

Doing the serialization on the consumer side is also an option that I though of. The gain on the database use may be minor since the consumer still need to check the permission, depending on how the permission are checked, for example, stateless token will not incur a DB check.

In the end, using the serializer directly in a modified ModelObserver was simpler since I already needed to modify _connect to dispatch the signal content on pre_delete instead of post_delete for my use case.

By the way, thank you for this library.

@hishnash hishnash changed the title [BUG] If Observer serializer is set, kwargs cause an unexpected argument error. [Improve Docs] Should include documentation for setting custom Observer serializer Feb 25, 2021
@hishnash hishnash added the good first issue Good for newcomers label Feb 25, 2021
@hishnash
Copy link
Member

One thing worth noting is depending on the channel layer you are using there might be a limit on the message size.

Just ensure the messages you encode are have predictable size you can be sure works with your selected channel layer.

If your using redis (most people are) then the size limit is quite large: 512 Megabytes (i would hope most peoples DRF sterilisers do not return this much data for a single object... ) but if you use something else Rabit the limit is 128MB however many cloud hosted rabbit systems might have much lower limits than these.

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

No branches or pull requests

2 participants