HA-423 - Fix Compass returning error for deleted GVL systems

[andres-torres-marroquin]

Closes HA-423

Description Of Changes

Fix Compass returning error for deleted GVL systems

Code Changes

• modified create_system in src/fides/api/db/system.py so model_dump doesn't use mode=json
• modified create_resource in src/fides/api/db/crud.py so a SQLAlchemyError exception logs.

Steps to Confirm

1. Try adding a compass system that has a vendor_deleted_date from the "add a system" form, eg AB InBev, more examples are on the jira ticket.

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
  • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
• Followup issues:
  • Followup issues created (include link)
  • No followup issues
• Database migrations:
  • Ensure that your downrev is up to date with the latest revision on main
  • Ensure that your downgrade() migration is correct and works
    • If a downgrade migration is not possible for this change, please call this out in the PR description!
  • No migrations
• Documentation:
  • Documentation complete, PR opened in fidesdocs
  • Documentation issue created in fidesdocs
  • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • No documentation updates required

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Feb 21, 2025 0:09am

[codecov]

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.22%. Comparing base (c4143a7) to head (f4b78e9).

Files with missing lines	Patch %	Lines
src/fides/api/db/crud.py	0.00%	2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (33.33%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5786      +/-   ##
==========================================
- Coverage   87.23%   87.22%   -0.01%     
==========================================
  Files         404      404              
  Lines       24678    24679       +1     
  Branches     2659     2659              
==========================================
- Hits        21528    21527       -1     
- Misses       2570     2571       +1     
- Partials      580      581       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adamsachs]

thanks for finding the issue @andres-torres-marroquin!

this may be an OK path to go down, but i've got some questions that we should clarify. generally, we need to add test coverage here - tests that surface the bug we're fixing, and i think also tests that prove we haven't caused a new regression with Url fields, as the comment suggests we may be.

relatedly, we need to make sure (ideally with automated tests) that update_system isn't being left with a similar bug to what you're fixing here...

[adamsachs]

nit: this isn't a very accurate description IMO, it's not compass that has any sort of error here

[adamsachs]

so did we validate that a resource here (i.e., a System) with an AnyUrl field that's populated actually gets created successfully? just trying to make sense of the comment that was clearly left here.

i'm also seeing that update_system in this module does a similar operation -

    existing_system_dict = copy.deepcopy(
        SystemSchema.model_validate(system)
    ).model_dump(mode="json")

is that not going to cause a similar problem? do we need to adjust that?