Refactor ModelObserver and Fix Test Warnings for Consistency and Safety

[tumblingman]

This PR includes three key updates to improve the functionality and reliability of the ModelObserver, the Observer unsubscribe logic, and the test suite:

1. Refactor ModelObserver for transaction safety:

   • Messages for database events (CREATE, UPDATE, DELETE) are now prepared before the transaction is committed.
   • Added new methods (prepare_messages, generate_messages, send_prepared_messages) to enhance code clarity and consistency.
   • Ensures DELETE actions retain access to the primary key (pk) and other instance data when messages are sent.

2. Fix unsubscribe logic in Observer:

   • Fixed a bug in the unsubscribe method of the BaseObserver class that caused the remove_group method to not be called under certain conditions due to incorrect logic.
   • Ensured proper cleanup of observer groups when unsubscribing, even when request_id is provided or groups become empty.
   • Covered this fix with unit tests to ensure the unsubscribe logic now works as expected in all scenarios.

3. Fix warnings and improve test suite:

   • Resolved warnings like Task was destroyed but it is pending! caused by improper cleanup in tests.
   • Added __test__ = False to test models to prevent pytest from misinterpreting them as test cases.
   • Overhauled communicator management in tests:
     • Introduced a context manager for the communicator to ensure proper cleanup.
     • Prevented dangling communicators from causing test instability.

Benefits:

• Improved transaction handling in ModelObserver for DELETE actions.
• Fixed a critical bug in the Observer unsubscribe logic, ensuring proper resource cleanup and reliability.
• Ensured all tests run cleanly without warnings or residual side effects.
• Simplified and standardized test communicator usage, reducing the risk of errors in future tests.

This PR significantly improves the reliability and maintainability of the ModelObserver implementation, Observer unsubscribe logic, and the associated test suite.

[hishnash]

What about making some larger changes to the class:

Do all the message building (for all actions) before the commit. And then on commit send them all.

This way the messages for delete will not be sent if the transaction is rolled back, but they will still have access to the pk.

You could have send_message yield or return the messages rather than calling https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3L166

Then have post_change_receiver do the same, then you can always call post_change_receiver before https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3R120 and then within connection.on_commit you can send them messages.

[tumblingman]

What about making some larger changes to the class:

Do all the message building (for all actions) before the commit. And then on commit send them all.

This way the messages for delete will not be sent if the transaction is rolled back, but they will still have access to the pk.

You could have send_message yield or return the messages rather than calling https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3L166

Then have post_change_receiver do the same, then you can always call post_change_receiver before https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3R120 and then within connection.on_commit you can send them messages.

Thank you for your suggestion! I really like the idea of handling all message building before the commit and then sending them on commit. It makes a lot of sense to ensure consistency and proper rollback handling. I'll work on implementing this approach tomorrow.

[tumblingman]

What about making some larger changes to the class:

Do all the message building (for all actions) before the commit. And then on commit send them all.

This way the messages for delete will not be sent if the transaction is rolled back, but they will still have access to the pk.

You could have send_message yield or return the messages rather than calling https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3L166

Then have post_change_receiver do the same, then you can always call post_change_receiver before https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3R120 and then within connection.on_commit you can send them messages.

hishnash, I'm done!

Pytest results:

[hishnash]

Looks like the tests are all timing out, you might need to bump some package versions to match your local setup.

https://github.com/NilCoalescing/djangochannelsrestframework/blob/master/setup.py. and in requirements.txt maybe would be good to pin these to a new version.

[tumblingman]

Looks like the tests are all timing out, you might need to bump some package versions to match your local setup.

https://github.com/NilCoalescing/djangochannelsrestframework/blob/master/setup.py. and in requirements.txt maybe would be good to pin these to a new version.

I’ve fixed everything and tested it again. I updated the list of dependencies and ran the tests once more. There were two errors in the tests: in two cases, after subscribing to database changes, instances of the model we are observing were immediately created, and the communicator was requested for a message. As a result, a TimeoutError occurred because the subscription process was not completed before the model creation.

Could you please let me know if the solution with a timeout works for you? Alternatively, in my code, I handle this differently: in every "subscribe" action, I always send a response to the client confirming that the subscription is established. This way, even in the tests, we can wait for a response before inserting data into the database to ensure that the observer works as expected. I believe this approach would be more deterministic.

Also, I want to note that during the first test run, before starting development, I encountered an error stating that the daphne package was missing in the virtual environment, and I had to install it manually. If you face a similar issue on the first run, maybe it makes sense to add daphne to requirements.txt?

[tumblingman]

In this latest commit, I refactored two test cases where multiple websocket connections are established, and one of them is terminated during execution.

Previously, the code used nested async with statements:

async with connected_communicator(TestOtherConsumer()) as communicator1:
    async with connected_communicator(TestUserConsumer()) as communicator2:

This was replaced with a more concise and flexible approach using AsyncExitStack:

async with AsyncExitStack() as stack:
    communicator1 = await stack.enter_async_context(connected_communicator(TestOtherConsumer()))
    communicator2 = await stack.enter_async_context(connected_communicator(TestUserConsumer()))

This change improves readability and offers greater flexibility for managing multiple connections, making it easier to modify or extend in future test scenarios.

@hishnash I think I’m done with all the updates and fixes. Tests are now passing in both the djangochannelsrestframework library and my own project where I use it. Please take a look at these latest changes, and let me know if there’s anything else to address. Thanks again for your time and patience!

Pytest results:

[tumblingman]

@hishnash hi!

I discovered a critical bug in the unsubscribe method of the BaseObserver class. The unsubscribe logic was not functioning as intended due to an issue where the remove_group method was not being called under certain conditions. This effectively meant that users of the library could not properly unsubscribe from observer groups, leading to potential resource leaks and unintended group subscriptions remaining active.

With the fix implemented in this PR, the unsubscribe logic now works as expected, ensuring proper cleanup of observer groups even when specific request_id values are provided or when groups become empty. This fix is crucial for anyone relying on dynamic subscriptions and unsubscriptions in their application, as it restores the intended functionality and reliability of the observer mechanism.

Please don’t be alarmed by the size of this PR (+1,366 −1,318), as the majority of the changes are due to indentation updates from refactoring tests to use asynchronous context managers consistently. The actual logic changes are around 100 lines, with a couple of hundred lines added for new test cases.

[tumblingman]

I see that some tests have failed, and this surprises me. To be honest, I only tested the code with Python 3.12 and Django 5. I’ll try to check it on other dependency versions in the coming days and make adjustments to the code or tests.

[tumblingman]

I identified the issue causing some tests to fail. The consumer was not unsubscribing from username: thenewname in time, likely due to slower machine performance during test execution. This resulted in the communicator receiving a message unexpectedly. To fix this, I increased the asyncio.sleep timeout to 2 seconds, ensuring sufficient time for the consumer to complete the unsubscribe action. This change should improve test reliability across different environments.

[tumblingman]

I fixed the tests to eliminate timeout errors. Now, whenever subscribing or unsubscribing to changes, we explicitly wait for the consumer's response before proceeding further in the test.

Please, run the test workflow in GitHub to verify the fixes.

[tumblingman]

To ensure test isolation and prevent signals from being received across tests, I added unique suffixes to group names in each test. This resolves the issue of overlapping group names causing unexpected behavior in asynchronous tests.

[tumblingman]

I have created PR #209, which is based on the changes introduced in this PR. It adds support for observing changes in many-to-many fields within the ModelObserver. Please note that PR #209 builds on top of #208, so once this is merged, the next step should be reviewing PR #209.