[py-tx] Deprecate SignalExchange API method #1773

b8zhong · 2025-02-26T23:04:44Z

Summary

Added deprecation notices to both methods in SignalExchangeAPI class, indicating they will be removed in version 2.x.x

Remove the few references to it there was.

Closes #1218

Test Plan

Yep

Dcallies

Thanks for taking a look!

Let's use the @deprecated decorator, and leave naive_fetch_merge around as a documentation of the correct way to merge this data.

Dcallies · 2025-03-04T15:43:33Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

        if new.deleted:
            return None


Hmm, this is a real functionality we need to store somewhere. The task mentions changing the behavior of fetch(). See L434 on the new code for where this is handled in convert

Dcallies · 2025-03-04T15:45:17Z

python-threatexchange/threatexchange/exchanges/signal_exchange_api.py

+        DEPRECATED: This method will be removed in version 2.x.x.
+


We should keep this version around - it's marked final, and the argument to keep it around is on L198.

Instead, we should should try and tell if it's safe to change this method not to call fetch_value_merge.

Suggested change

DEPRECATED: This method will be removed in version 2.x.x.

Dcallies · 2025-03-04T15:48:25Z

python-threatexchange/threatexchange/exchanges/signal_exchange_api.py

@@ -167,6 +167,10 @@ def fetch_value_merge(
        """
        Merge a new update produced by fetch.

+        DEPRECATED: This method will be removed in version 2.x.x.


In addition, we should use the @deprecated annotation - you can include the removal message there.

https://typing.readthedocs.io/en/latest/spec/directives.html#deprecated

@Dcallies py-tx goes back to Python 3.8 right? @deprecated I think is only introduced in 3.13

What do you think of using warnings.warn?

D'oh! It goes back to 3.9 (3.8 is deprecated).

Warnings is a fine workaround.

Dcallies · 2025-03-04T15:48:58Z

python-threatexchange/threatexchange/exchanges/helpers.py

@@ -56,7 +56,13 @@ def delta(self, value: fetch_state.FetchDeltaTyped) -> None:
            self._delta = value
        else:
            old = self._delta.updates
-        self.api_cls.naive_fetch_merge(old, value.updates)


Let's keep this version around, it's useful to formally describe the update logic somewhere.

…alExchangeAPI

b8zhong · 2025-03-11T22:09:02Z

python-threatexchange/threatexchange/exchanges/signal_exchange_api.py

@@ -197,11 +210,10 @@ def naive_fetch_merge(
        together keyed by ID will eventually get you an entire copy of the database.
        """
        for k, v in new.items():
-            new_v = cls.fetch_value_merge(old.get(k), v)
-            if new_v is None:
+            if v is None:


I think... this might be right? If something is deleted -- it would already be None... ?

If we were outright deleting fetch_value_merge, then yes!

However, we might need to keep the old code around until 2.x.x. See toplevel comment about what to do.

Dcallies

Hmm, as written I think this might break the NCMEC fetch code! We might be missing a unittest that tests to make sure NCMEC deletions are handled properly (or else it might be a bug that is compensated for later).

Let's break this up into stages:

Review the current API integrations and see which ones are relying on an override of fetch_value_merge
Fix the implementations relying on fetch_value_merge to not do so, which allows for the deletion of the override on that exchange class
In order to defend against the existence of any third party extensions that rely on overrides, you could try detecting whether the class has an override which is not equal to the default. I think you might be able to determine that via reflection. If you detect an override, emit a warning.

Dcallies · 2025-03-12T20:13:29Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

+        """
+        warnings.warn(
+            "fetch_value_merge() is deprecated and will be removed in version 2.x.x. "
+            "The deletion logic has been moved directly to fetch_iter().",


Not quite - you need to add this logic in fetch_iter!

Dcallies · 2025-03-12T20:14:08Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

+                    {
+                        f"{update.member_id}-{update.id}": update
+                        for update in entry.updates
+                    },


Instead of yielding : update, you need to yield None if update is deleted - you could inline it here.

Dcallies · 2025-03-12T20:14:57Z

python-threatexchange/threatexchange/exchanges/signal_exchange_api.py

@@ -167,6 +167,10 @@ def fetch_value_merge(
        """
        Merge a new update produced by fetch.

+        DEPRECATED: This method will be removed in version 2.x.x.


D'oh! It goes back to 3.9 (3.8 is deprecated).

Warnings is a fine workaround.

Dcallies · 2025-03-12T20:16:45Z

python-threatexchange/threatexchange/exchanges/signal_exchange_api.py

+        warnings.warn(
+            "fetch_value_merge() is deprecated and will be removed in version 2.x.x. "
+            "Any implementations that convert records into None to signal a deletion "
+            "should instead move that logic into the fetch() method.",


I think we figured out it's fetch_iter

Dcallies · 2025-03-12T20:19:34Z

python-threatexchange/threatexchange/exchanges/signal_exchange_api.py

@@ -197,11 +210,10 @@ def naive_fetch_merge(
        together keyed by ID will eventually get you an entire copy of the database.
        """
        for k, v in new.items():
-            new_v = cls.fetch_value_merge(old.get(k), v)
-            if new_v is None:
+            if v is None:


If we were outright deleting fetch_value_merge, then yes!

However, we might need to keep the old code around until 2.x.x. See toplevel comment about what to do.

b8zhong added 2 commits February 26, 2025 17:59

Deprecate SignalExchangeAPI

c5d5617

feat: lint

b7c2002

b8zhong requested a review from Dcallies as a code owner February 26, 2025 23:04

facebook-github-bot added the CLA Signed label Feb 26, 2025

b8zhong changed the title ~~1218 deprecate signal exchange api~~ [py-tx] Deprecate SignalExchange API method Feb 26, 2025

b8zhong added 3 commits February 26, 2025 18:10

lint: black again

a9b72a6

fix some tests

77c9ccc

final black check..

42cdd3a

Dcallies requested changes Mar 4, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 1218-deprecate-Sign…

e9cd9c8

…alExchangeAPI

github-actions bot added the python-threatexchange Items related to the threatexchange python tool / library label Mar 11, 2025

b8zhong added 3 commits March 11, 2025 17:58

address comments, use warnings.warn

30d8a44

address comments, use warnings.warn

bacaf41

address comments, use warnings.warn

2e44ca3

b8zhong commented Mar 11, 2025

View reviewed changes

b8zhong requested a review from Dcallies March 11, 2025 22:09

Dcallies requested changes Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[py-tx] Deprecate SignalExchange API method #1773

[py-tx] Deprecate SignalExchange API method #1773

b8zhong commented Feb 26, 2025

Dcallies left a comment

Dcallies Mar 4, 2025

Dcallies Mar 4, 2025

Dcallies Mar 4, 2025

b8zhong Mar 11, 2025

Dcallies Mar 12, 2025

Dcallies Mar 4, 2025

b8zhong Mar 11, 2025

b8zhong Mar 11, 2025

Dcallies Mar 12, 2025

Dcallies left a comment

Dcallies Mar 12, 2025

Dcallies Mar 12, 2025

Dcallies Mar 12, 2025

Dcallies Mar 12, 2025

Dcallies Mar 12, 2025

[py-tx] Deprecate SignalExchange API method #1773

Are you sure you want to change the base?

[py-tx] Deprecate SignalExchange API method #1773

Conversation

b8zhong commented Feb 26, 2025

Summary

Test Plan

Dcallies left a 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

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

Dcallies left a 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