System integration link: address PR review follow-ups#7550
Conversation
- Remove lazy-load footgun in from_orm; pass system info explicitly via SQL joins - Return lightweight ref entities from resolve_connection_config/resolve_system - Add SystemLinkResponse.from_entity() to reduce route handler duplication - Move TooManyLinksError check after connection config lookup in set_links - Remove empty __table_args__ from SystemConnectionConfigLink model - Remove unnecessary cleanup_links autouse fixtures from tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ype annotation - Use outerjoin instead of inner join so links with deleted systems are still returned (with None system info) rather than silently dropped - Make _to_entity_with_system an instance method (called via self) - Remove unnecessary string forward reference on SystemRef type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR is a clean code-quality follow-up to #7432, addressing reviewer feedback with no intended behavioral changes. It eliminates the lazy-load footgun by passing system info explicitly via SQL joins, introduces lightweight Key changes:
Confidence Score: 4/5
Last reviewed commit: 7d3a552 |
| def _to_entity_with_system( | ||
| self, link: SystemConnectionConfigLink, *, session: Session | ||
| ) -> SystemIntegrationLinkEntity: | ||
| """Convert an ORM link to an entity, fetching system info via a join.""" | ||
| stmt = select(System.fides_key, System.name).where(System.id == link.system_id) | ||
| row = session.execute(stmt).first() | ||
| return SystemIntegrationLinkEntity.from_orm( | ||
| link, | ||
| system_fides_key=row.fides_key if row else None, | ||
| system_name=row.name if row else None, | ||
| ) |
There was a problem hiding this comment.
_to_entity_with_system should be a @staticmethod
This method does not reference self anywhere in its body — it operates entirely on the link and session parameters. Per project convention, methods that don't require instance state should be declared as @staticmethod.
Adding @staticmethod and dropping self from the signature would make this explicit. The existing call sites (e.g. self._to_entity_with_system(link, session=session)) will continue to work unchanged since Python allows calling static methods through instances.
Context Used: Rule from dashboard - Use static methods instead of instance methods when the method doesn't require instance state. (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
noted. this general point is coming into conflict with our standard repository pattern, so we do need to reconcile that more broadly. but i think this approach is consistent with the repository class as it stands now, so i'm OK keeping this as-is.
📝 WalkthroughWalkthroughAdds lightweight reference dataclasses and changes entity construction to accept explicit system metadata; repository methods now return refs and construct entities with provided system keys; response factory added; minor ORM/typing tweaks; autouse test cleanup fixtures removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/system_integration_link/schemas.py`:
- Around line 26-31: from_entity can pass None into
SystemLinkResponse.system_fides_key (a required str) when the joined System is
missing; update the from_entity method to ensure a string is always provided by
replacing system_fides_key=entity.system_fides_key with a safe fallback (e.g.,
system_fides_key=entity.system_fides_key or "") so Pydantic validation never
fails for outer-joined rows; refer to the from_entity method,
SystemIntegrationLinkEntity, and SystemLinkResponse when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c0a70fd-9a3f-407f-9141-0e5d8c707ba4
📒 Files selected for processing (9)
src/fides/system_integration_link/entities.pysrc/fides/system_integration_link/models.pysrc/fides/system_integration_link/repository.pysrc/fides/system_integration_link/routes.pysrc/fides/system_integration_link/schemas.pysrc/fides/system_integration_link/service.pytests/system_integration_link/test_repository.pytests/system_integration_link/test_routes.pytests/system_integration_link/test_service.py
💤 Files with no reviewable changes (4)
- tests/system_integration_link/test_service.py
- src/fides/system_integration_link/models.py
- tests/system_integration_link/test_routes.py
- tests/system_integration_link/test_repository.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/system_integration_link/schemas.py`:
- Around line 25-31: The created_at type on SystemIntegrationLinkEntity
currently mismatches the DB constraint and SystemLinkResponse: update
SystemIntegrationLinkEntity.created_at to be non-optional (created_at: datetime)
so it matches the ORM column (nullable=False, server_default=func.now()) and the
SystemLinkResponse.created_at: datetime, or alternatively change
SystemLinkResponse.created_at to Optional[datetime] if you intend created_at to
be nullable; adjust the from_entity classmethod in SystemLinkResponse
accordingly to reflect the chosen non-optional/optional type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb929453-ed57-476f-b4ab-328a269c3b4d
📒 Files selected for processing (1)
src/fides/system_integration_link/schemas.py
These fields map to NOT NULL DB columns with server_default, so they are always populated. Aligns the entity type with the DB constraint and the SystemLinkResponse schema. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
JadeCara
left a comment
There was a problem hiding this comment.
One super small comment, overall very clean. Approved
|
|
||
| class SystemLinkResponse(BaseModel): | ||
| system_fides_key: str | ||
| system_fides_key: Optional[str] = None |
There was a problem hiding this comment.
I am not sure this should be Optional. This alters the OpenAPI contract, since clients that previously expected a required string field will now see it as optional. In practice this should never be None (the FK is NOT NULL and the system FK has ondelete="CASCADE", so the outerjoin always finds a row).
There was a problem hiding this comment.
This is minor - totally understand the type mismatching.
There was a problem hiding this comment.
ha, the funny thing is i made it optional because of a previous AI code review comment that it technically could be empty because we're doing an outer join on the underlying query, and the only reason i switched to doing an outer join was because of an AI ask that we make the query more 'defensive'...
so we've really got these LLMs talking in circles. honestly i just think i shoudl go back to my original implementation that had an inner join, because as the comment here points out - we're cascading deletes, and the whole point of the join record is to actually join records...
The FK is NOT NULL with ondelete=CASCADE, so the joined System row is always present. Switch outerjoin → join, remove None fallbacks, and make system_fides_key non-optional across entity, schema, and repository. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fides/system_integration_link/repository.py (1)
154-164: Consider defensive handling for missing system.The
.one()call on line 159 will raiseNoResultFoundif the system doesn't exist. While the FK constraint withCASCADEdelete should prevent orphaned links, there's a brief TOCTOU window after flush but before commit where the system could be deleted by a concurrent transaction.For a write operation this risk is minimal, but you could consider using
.one_or_none()with an explicit error message for easier debugging if this ever occurs.♻️ Optional: Defensive handling with clearer error
def _to_entity_with_system( self, link: SystemConnectionConfigLink, *, session: Session ) -> SystemIntegrationLinkEntity: """Convert an ORM link to an entity, fetching system info via a join.""" stmt = select(System.fides_key, System.name).where(System.id == link.system_id) - row = session.execute(stmt).one() + row = session.execute(stmt).one_or_none() + if row is None: + raise RuntimeError( + f"System {link.system_id} not found for link {link.id}" + ) return SystemIntegrationLinkEntity.from_orm( link, system_fides_key=row.fides_key, system_name=row.name, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/system_integration_link/repository.py` around lines 154 - 164, The current _to_entity_with_system method uses session.execute(...).one() which will raise NoResultFound if System row is missing; change it to use .one_or_none() (or .first()) when executing the select(System.fides_key, System.name).where(System.id == link.system_id) and explicitly handle a None result by raising a clear, descriptive error (or returning a well-defined fallback) before calling SystemIntegrationLinkEntity.from_orm; reference the symbols _to_entity_with_system, System, SystemIntegrationLinkEntity, and session.execute/.one() so the change is applied in that conversion helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fides/system_integration_link/repository.py`:
- Around line 154-164: The current _to_entity_with_system method uses
session.execute(...).one() which will raise NoResultFound if System row is
missing; change it to use .one_or_none() (or .first()) when executing the
select(System.fides_key, System.name).where(System.id == link.system_id) and
explicitly handle a None result by raising a clear, descriptive error (or
returning a well-defined fallback) before calling
SystemIntegrationLinkEntity.from_orm; reference the symbols
_to_entity_with_system, System, SystemIntegrationLinkEntity, and
session.execute/.one() so the change is applied in that conversion helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2747d440-22f0-4fc5-8b72-34ca1dc7b1f0
📒 Files selected for processing (3)
src/fides/system_integration_link/entities.pysrc/fides/system_integration_link/repository.pysrc/fides/system_integration_link/schemas.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fides/system_integration_link/schemas.py
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Ticket ENG-2590
Description Of Changes
Address follow-up items from @galvana's code review on #7432 (system integration link). These are code quality improvements with no behavioral changes:
from_ormno longer accessesobj.system(which could trigger an implicit lazy load). System info is now passed explicitly via SQL joins in the repository layer.resolve_connection_configandresolve_systemnow return lightweightConnectionConfigRef/SystemRefdataclasses instead of ORM models, preventingDetachedInstanceErrorif called without an explicit session.SystemLinkResponse.from_entity()classmethod, used in bothget_system_linksandset_system_links.TooManyLinksErrorcheck after connection config lookup inset_links, so a missing connection returns 404 instead of 400.__table_args__fromSystemConnectionConfigLinkmodel and unnecessarycleanup_linksautouse test fixtures.Code Changes
src/fides/system_integration_link/entities.py- AddedConnectionConfigRefandSystemRefdataclasses; updatedfrom_ormto accept explicit system info paramssrc/fides/system_integration_link/repository.py- Read methods use SQL joins instead ofjoinedload; write methods use_to_entity_with_systemhelper; resolve methods return ref entitiessrc/fides/system_integration_link/routes.py- UseSystemLinkResponse.from_entity()in both handlerssrc/fides/system_integration_link/schemas.py- AddedSystemLinkResponse.from_entity()classmethodsrc/fides/system_integration_link/service.py- Reordered validation: connection config lookup before length checksrc/fides/system_integration_link/models.py- Removed empty__table_args__tests/system_integration_link/test_repository.py- Removedcleanup_linksautouse fixture and unused importtests/system_integration_link/test_routes.py- Removedcleanup_linksautouse fixture and unused importtests/system_integration_link/test_service.py- Removedcleanup_linksautouse fixture and unused importSteps to Confirm
system_integration_linktests pass without modification (behavior unchanged)Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
Refactor
New Features
Tests