Skip to content

[hma][api] bank_get_content can optionally return signals #1763

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

vincent-4
Copy link
Contributor

@vincent-4 vincent-4 commented Feb 13, 2025

Summary

An attempt:
Not 100% on the DB stuff. I'm having issues running tests locally, so I'm mainly putting this up for now to see the CI workflow output. If there are failures, feel free to come back later, when it would make more sense.

Test Plan

Yes, new tests in PR: tests are pretty self-explanatory, in this case at least.
Edit: This is woards #1761

Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this is a toughie, so bear with me. Thanks for testing.

My primary concerns are:

  1. Making sure we don't degrade performance for the matching usecase, which doesn't need signals
  2. Making sure the solution solves the usecase for the folks who need [hma] Add the ability to GET the signals from a content_id #1761
  3. Come up with a simple solution

I think after noodling on it a bit, I think I am convinced that folks will be wanting to do the fetch on signals at the same time they are getting banked_content, which is how you've implemented it in the interface. My recommendation is to adjust the return value of the get_signals interface to return those as separate components to handle this, and for the second component to be none/empty if not requested.

However, I believe this PR is always fetching the signals (I believe via a hidden interaction on the property for .signals), even if we are going to ignore them later, so I think we need to adjust the postgres impl to make sure we aren't fetching them.

On the API interface itself for getting banked content, please make it so the key for "signals" does not appear if the user hasn't requested it - it's confusing to have it empty (or populated due to cost) if it's not requested.

EDIT: Whoops, forgot to hit request changes.

I think your base is overall solid, and since your unittest is on the API interface you don't have to change much on that side.

doesn't auto format on save work?
maybe be more faithful to the original stuff, but we can revert
format
something different oought to do it?
@vincent-4 vincent-4 force-pushed the hma/get-signals-from-content-id branch 2 times, most recently from 07a9aa8 to 5532174 Compare February 27, 2025 22:16
see if we can match the tests, and change the query type to resolve test failures
…e query itself: might fix the actual tests failure
update some commentary and the query to only fetch when requested

Fix the type

only if requested

this does it?

this only updates but shouldn't this pass?

remove a redundant test
@vincent-4 vincent-4 force-pushed the hma/get-signals-from-content-id branch from 5532174 to 4b88738 Compare February 27, 2025 22:17
@vincent-4
Copy link
Contributor Author

I haven't hit resolve on a few blocking comments that I don't have a particular opinion on. I think this is ready for a second look, if you're able to.

I think, maybe I could make the interface changes in another PR, either separate or combined with the second needed one about more of the nitty gritty implementation.

@Dcallies
Copy link
Contributor

Dcallies commented Apr 9, 2025

Hey @vincent-4! I lost track of this one, let me self-put it up for review, and I might do some work to split out some of the changes to help land it, sorry for the delay!

@Dcallies Dcallies self-requested a review April 9, 2025 17:54
@Dcallies Dcallies changed the title [hma] first stab at implementing endpoint [hma][api] bank_get_content can optionally return signals Apr 23, 2025
Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @vincent-4 - sorry for the long delay here, I lost the thread on this one.

I think the functionality presented is fine, and just think we should very slightly tweak the interface. I am happy to bring this one in for a landing, since it's been literal months since we talked.

I have a meeting with some of the users later today, and based on that I may commandeer this one and finish these comments to land.

@github-actions github-actions bot added the hma Items related to the hasher-matcher-actioner system label Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed hma Items related to the hasher-matcher-actioner system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants