ENG-3015: Add event listener warning on ConnectionConfig.secrets#7736
ENG-3015: Add event listener warning on ConnectionConfig.secrets#7736
Conversation
fidesplus registers SQLAlchemy attribute events on this column for Jira credential auto-sync. This comment warns future developers to use ORM instance-level updates (not bulk/raw SQL) so events fire. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Greptile SummaryThis PR adds a 4-line developer-facing warning comment above the
Confidence Score: 5/5
Important Files Changed
Reviews (1): Last reviewed commit: "Add event listener warning comment on Co..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review — ENG-3015
This is a comment-only change (4 lines added, 0 deleted) so no functional or security concerns. The note is well-placed, accurate, and will help prevent a subtle cross-repo breakage.
Overall
The comment is clear and covers the essential points: what is listening, why bulk/raw SQL breaks it, and where to find the fidesplus implementation. Good defensive documentation.
Suggestions
Scope of the warning: The comment mentions Jira SaaS → jira_ticket as the current consumer, but if this event-listener pattern is (or becomes) used for other connection types in fidesplus, that parenthetical could become misleading. Worth considering whether "cross-connection credential sync" alone is sufficient without naming the specific integration — or phrasing it as "currently used for…" to signal it may grow.
MutableDict vs. raw SQL distinction: MutableDict.as_mutable(...) already handles in-place dict mutation tracking at the ORM layer, so the real risk this comment is guarding against is bypassing the ORM entirely (e.g., session.execute(update(...)) or Alembic data migrations). The comment correctly says "bulk/raw SQL updates", but calling out "Alembic data migrations" explicitly might make it even clearer for the developer most likely to trip over this.
Nice to Have
The fidesplus path (fidesplus/jira/jira_credential_sync.py) won't be navigable from this repo. Adding a one-line note that the referenced file lives in the fidesplus repo (not this one) would save a future developer a few minutes of searching.
Summary
No issues that block merge. The comment is the right mitigation for protecting a non-obvious cross-repo invariant. The suggestions above are optional polish.
Summary
ConnectionConfig.secretswarning that fidesplus registers SQLAlchemy attribute events on this column for Jira credential auto-syncContext
Companion to fidesplus#3252 which adds cross-connection credential sync via SQLAlchemy attribute events. This comment is the cheapest, highest-leverage mitigation against silent sync breakage if someone later adds bulk or raw SQL updates to the secrets column.
Test plan
🤖 Generated with Claude Code