-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue #2603] Setup triggers on our opportunity tables which populate the search queue table #2611
[Issue #2603] Setup triggers on our opportunity tables which populate the search queue table #2611
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I didn't catch on your last PR - but that we need before we can merge this.
We need the Opportunity ORM model to be aware of its record in the queue table otherwise we can't delete it.
You can see this if you run make console
and then try to do:
x = dbs.query(Opportunity).first()
dbs.delete(x)
dbs.commit()
which errors with:
sqlalchemy.exc.IntegrityError: (psycopg.errors.ForeignKeyViolation) update or delete on table "opportunity" violates foreign key constraint "opportunity_search_index_queue_opportunity_id_opportunity_fkey" on table "opportunity_search_index_queue"
We solve this with other foreign keys with the cascade="all, delete-orphan"
on the relationship.
Alternatively.. I suppose there isn't anything stopping us from making this whole "has_update" column just exist in the opportunity table itself. Thoughts?
api/src/db/migrations/versions/2024_10_28_add_opportunity_table_triggers.py
Outdated
Show resolved
Hide resolved
…e_triggers.py Co-authored-by: Michael Chouinard <[email protected]>
I kind of like the idea of a separate table to handle the concern of the updates, which can expand if we need to store other info to store into the search queue later on. For this reason I think going the
|
…s://github.com/HHS/simpler-grants-gov into mikehgrantsgov/2603-opportunity-table-triggers
opportunity: Mapped[Opportunity] = relationship( | ||
Opportunity, cascade="all, delete-orphan", single_parent=True | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need this relationship to go the other way. Doing this would delete the opportunity if you deleted the queue record.
Basically the same as current_opportunity_summary in the opportunity model:
current_opportunity_summary: Mapped["CurrentOpportunitySummary | None"] = relationship(
back_populates="opportunity", single_parent=True, cascade="all, delete-orphan"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good call. I thought the single_parent
property would have done this on the queue. Updated this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary
Fixes #2603
Time to review: 15 mins
Changes proposed
Add migration which adds DB triggers to populate
opportunity_search_index_queue
based on updates made to existing tables:opportunity
opportunity_assistance_listing
current_opportunity_summary
opportunity_summary
link_opportunity_summary_funding_instrument
link_opportunity_summary_funding_category
link_opportunity_summary_applicant_type
opportunity_attachment
Context for reviewers
See test SQL below. Created migration will add or update entries in the opportunity_search_index_queue table until a subsequent process handles them.
Additional information
See attached SQL file for running tests based on these changes: