Allow to add context to payload #73

romank0 · 2024-02-08T12:46:34Z

Here's an extract from the docs:

Sometimes it is beneficial to pass some contextual information from the trigger
to the trigger listener along the payload. Examples are:

tracing information that allows to track complex request processing in a multi component system
in a multitenant system a tenant information to be able to identify the tenant that peformed an operation that triggered a notification

This can be done by using Payload Context. This feature includes:

ability to add an additional information to the payload in the trigger
ability to filter by the fields in the context in the listener process
ability to use context fields in the listener callbacks

Note that this is based on #66 and incorporates that PR.

PaulGilmartin · 2024-02-11T21:51:29Z

Thanks @romank0. I will do my best to look at this and your other PRs this coming week.

PaulGilmartin · 2024-02-14T13:41:33Z

@romank0 Now that #66 is merged, if you rebase I can take a look at this one

…stream

PaulGilmartin · 2024-02-14T15:35:43Z

docs/api.rst

@@ -18,6 +18,9 @@ Channels
 Listeners
 ---------



Just a heads up that I'm currently having troubles deploying updates to readthedocs. The webhook was accidentally removed, and there seems to be no way of getting it back as django-pgpubsub does not appear as a project on my personal readthedocs (even after transferring this library to my own github. I'm trying to get the owner of Opus10 to help me out here, but until then we won't get readthedocs updates unfortunately.

PaulGilmartin · 2024-02-14T15:39:25Z

docs/payload_context.rst

+    set_notification_context({'some-key': 'some-value'})
+
+The setting is effective till the connection is closed. Alternatively the setting
+``PGPUBSUB_TX_BOUND_NOTIFICATION_CONTEXT`` can be used to clean the context at the end 


I don't fully understand the naming PGPUBSUB_TX_BOUND_NOTIFICATION_CONTEXT here. What does "TX_BOUND" mean? I guess it means "transaction bound" - maybe just a comment in the docs explaining that. Should we also mention what kind of values this setting can take? It makes sense it's a boolean, but when we see context being set as a dict just above, it could be confusing to some.

TX_BOUND means "bound to the transaction" as oposed to bound to the connection. When the context is bound to the transaction it is cleared in the end of the transaction. This config param is a boolean, I'll update this document.

TX is common abbreviation for transaction, I can rename this to be PGPUBSUB_NOTIFICATION_CONTEXT_BOUND_TO_TRANSACTION if you think this is better.

PaulGilmartin · 2024-02-14T15:41:45Z

docs/payload_context.rst

+Filter by ``context`` field in the trigger listener
+---------------------------------------------------
+
+Define a class that implements ``ListenerFilterProvider`` protocol and set


I think it should be "implements the ..."

PaulGilmartin · 2024-02-14T15:52:50Z

docs/payload_context.rst

+            return Q(payload__context__tenant='my-tenant')
+
+    # django settings
+    PGPUBSUB_LISTENER_FILTER = 'myapp.whatever.TenantListenerFilterProvider'


Does this implicitly imply we can only have one filter provider at a time? If so that seems a little restrictive - I would think there could be many cases that we'd want one listener to filter by one key/value, and another by another key/value. I can see how this could be a good global filter if we wanted one, so it's a good start in that regard.

I think a bit more of versatile API would be something like

@pgpubsub.post_insert_listener(AuthorTriggerChannel, context_filter=TenantListenerFilterProvider()) def create_first_post_for_author( old: Author, new: Author, context: Dict[str, Any] ):

Such an API would allow different filters per listener. It could also be used to imply that listener with non null context_filter is automatically accepting contexts, hence removing the need for PGPUBSUB_PASS_CONTEXT_TO_LISTENERS. Of course PGPUBSUB_PASS_CONTEXT_TO_LISTENERS is good as a global back up as is the other setting.

I guess we can think of this is an addition to what we have here, so not a blocker. Just thought I'd mention to see what you think?

That's true that the filter provider is global and the current implementation is restrictive in this regard. I don't see a use-case for non global filter provider though.

The only use case I have at hand and the motivation for this PR is the deployments where one DB is shared by multilple tenants but each tenant has it's own listener process and needs to process only notifications about changes done by that tenant. For this scenario the global filter provider works fine. In order to generalize this I need at least one real life example how multiple filter providers would be used to design a convenient API.

Re PGPUBSUB_PASS_CONTEXT_TO_LISTENERS I think it is possible to get rid of it completely by analyzing the signature of the listener callback but I haven't tried that.

PaulGilmartin · 2024-02-14T15:56:31Z

docs/payload_context.rst

+
+    class TenantListenerFilterProvider(ListenerFilterProvider):
+        def get_filter(self) -> Q:
+            return Q(payload__context__tenant='my-tenant')


This will all only work for channels with lock_notifications right (i.e. with stored notifications)? Should we mention that somewhere?

That's a good point.

PaulGilmartin · 2024-02-14T16:00:59Z

pgpubsub/channel.py

+            else:
+                scope = 'SESSION'
+            cursor.execute(
+                f'SET {scope} pgpubsub.notification_context = %s',


I'm a little bit confused here so probably a stupid question. What is actually being set here - where does pgpubsub.notification_context live in the db?

This is a custom parameter. They are like custom defined settings that are stored either for a connection (scope='SESSION') or for a transaction (scope='LOCAL').

PaulGilmartin · 2024-02-14T16:01:48Z

pgpubsub/models.py

@@ -1,4 +1,4 @@
-from typing import Type
+from typing import Optional, Type


Unused import?

yep, removing

PaulGilmartin · 2024-02-14T16:07:49Z

pgpubsub/triggers.py

@@ -26,6 +30,11 @@ def _build_payload(self, model):
            payload := '{{"app": "{model._meta.app_label}", "model": "{model.__name__}"}}'::jsonb;
            payload := jsonb_insert(payload, '{{old}}', COALESCE(to_jsonb(OLD), 'null'));
            payload := jsonb_insert(payload, '{{new}}', COALESCE(to_jsonb(NEW), 'null'));
+            SELECT current_setting('pgpubsub.notification_context', True) INTO notification_context_text;


Again, for some reason I'm confused what pgpubsub.notification_context is here (and also what current_setting is).

current_setting is a built in function (see docs) that allows to get the value of the setting. In this case the custom one named pgpubsub.notification_context.

PaulGilmartin · 2024-02-14T16:09:29Z

pgpubsub/triggers.py

+        return [
+            ('payload', 'JSONB'),
+            ('notification_context', 'JSONB'),
+            ('notification_context_text', 'TEXT'),


Do we need both for the backwards compatibility reason?

No, it's just an oversight from my side. I'll remove unused variable

PaulGilmartin · 2024-02-14T16:12:05Z

@romank0 Thanks for another great PR. I've left some comments, mainly me just trying to understand and clarify some things.

I'm thinking with an addition of a feature this size and with the help you provide in fixing bugs, it makes sense to list you as a primary author of the library and also to give you more privileges on the repo to help maintain it. If that's something you'd be interested in, please let me know and I'll see what options there are.

Roman Konoval added 16 commits February 8, 2024 13:07

allows to store and filter by payload extras

563b822

listener filter as fully qualified class

f0498d3

option to pass extras to the listener

1a25627

cleanup

1777b0f

adds docs

19241e8

removes unused mark

621c3a0

removes unused markers

c0cc4df

removes unused import

31e9259

fixes typos

f66011e

changes to payload context storage

afa7014

handles empty context setting

0350b76

ignores tx aborted error

1f7cdbd

adds filter by payload context to listener recovery

4afba93

check that there tx is ok from django perspective

f51c288

adds support for session bound notification context

0c1ed90

document PGPUBSUB_TX_BOUND_NOTIFICATION_CONTEXT option

645caa0

romank0 mentioned this pull request Feb 8, 2024

Do not send the complete payload for the TriggerChannel that stores notification in the DB #67

Open

Merge remote-tracking branch 'upstream/master' into payload-extras-up…

3908bae

…stream

PaulGilmartin reviewed Feb 14, 2024

View reviewed changes

fixes review comments

1e1f648

PaulGilmartin approved these changes Feb 15, 2024

View reviewed changes

PaulGilmartin merged commit 5e8c45f into PaulGilmartin:master Feb 15, 2024
1 check passed

romank0 deleted the payload-extras-upstream branch February 15, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to add context to payload #73

Allow to add context to payload #73

romank0 commented Feb 8, 2024 •

edited

Loading

PaulGilmartin commented Feb 11, 2024

PaulGilmartin commented Feb 14, 2024

PaulGilmartin Feb 14, 2024

PaulGilmartin Feb 14, 2024 •

edited

Loading

romank0 Feb 14, 2024

romank0 Feb 14, 2024

PaulGilmartin Feb 14, 2024

PaulGilmartin Feb 14, 2024

romank0 Feb 14, 2024 •

edited

Loading

PaulGilmartin Feb 14, 2024

romank0 Feb 14, 2024

PaulGilmartin Feb 14, 2024

romank0 Feb 14, 2024

PaulGilmartin Feb 14, 2024

romank0 Feb 14, 2024

PaulGilmartin Feb 14, 2024 •

edited

Loading

romank0 Feb 14, 2024

PaulGilmartin Feb 14, 2024

romank0 Feb 14, 2024

PaulGilmartin commented Feb 14, 2024

		@@ -1,4 +1,4 @@
		from typing import Type
		from typing import Optional, Type

Allow to add context to payload #73

Allow to add context to payload #73

Conversation

romank0 commented Feb 8, 2024 • edited Loading

PaulGilmartin commented Feb 11, 2024

PaulGilmartin commented Feb 14, 2024

Choose a reason for hiding this comment

PaulGilmartin Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romank0 Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulGilmartin Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulGilmartin commented Feb 14, 2024

romank0 commented Feb 8, 2024 •

edited

Loading

PaulGilmartin Feb 14, 2024 •

edited

Loading

romank0 Feb 14, 2024 •

edited

Loading

PaulGilmartin Feb 14, 2024 •

edited

Loading