Eng 567 pr2 saas config version

[Vagoasdf]

Ticket []

Description Of Changes

Setting up the backend for Saas Config historic information

This PR sets up the backend for the historical record keeping of saas integrations, creating two new models, SaaSConfigVersion for storing version changes of out of the box integrations, and ConnectionConfigSaaSHistory, to store the history of a specific instance of a saas connection. It also creates the endpoints that would be used to request data from these history versions

Code Changes

• Creates new Models for storing saas config version
• Creates Migrations for said models
• Set up Endpoints to access them

Steps to Confirm

1. Start up a Server
2. Check on the localhost API that the out of the box integrations have set up their version template trought /connector-templates/{connector_template_type}/versions
3. Create a new Integration connection
4. Call to the /connection/{connection_key}/saas-history and check that the saas history is recorded

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
  • Updates unreleased work already in Changelog, no new entry necessary
• UX feedback:
  • All UX related changes have been reviewed by a designer
  • No UX review needed
• Followup issues:
  • Followup issues created
  • 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 GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ignored	Preview	Mar 27, 2026 7:52pm
fides-privacy-center	Ignored		Mar 27, 2026 7:52pm

[claude]

Code Review Summary

Good feature overall — the two-table design (template-level SaaSConfigVersion vs. connection-scoped ConnectionConfigSaaSHistory) is well-motivated and the append-only history approach is sound. Test coverage for the connection-history endpoints is thorough. A few things need attention before merge:

Critical (Must Fix)

• .mcp.json should not be committed — contains hardcoded local machine paths (/Users/bruno/Ethyca/...). Add to .gitignore instead.
• Unused imports — load_config_from_string and load_dataset_from_string are imported in connector_template_endpoints.py but never used.
• Missing tests — the three new connector-template version endpoints (/versions, /versions/{version}/config, /versions/{version}/dataset) have no test coverage.

Suggestions

• Model method doing cross-model work — update_saas_config now imports DatasetConfig and ConnectionConfigSaaSHistory, queries related rows, and creates a snapshot. This is heavy for a model method and makes it hard to test in isolation. Consider moving the orchestration to a service layer.
• Business logic in the route handler — SaaSConfigVersion.upsert is called directly inside patch_saas_config. This belongs in a service or co-located with update_saas_config.
• Double ConnectorRegistry.get_connector_template call — in patch_saas_config the template can be fetched twice. Fetching once before the if not existing_saas_config block and reusing the result is cleaner.
• Missing index on connection_key — the column is denormalized specifically so history survives connection deletion, but no index backs it up.
• DB queries in the endpoint file — _get_version_row and the inline queries in list_connector_template_versions should live in a service or repository, not directly in the endpoint module.
• Startup queries one row per connector per boot — sync_oob_saas_config_versions can be made more efficient by fetching all existing (connector_type, version) pairs in one query and only inserting the missing ones.

Nice to Have

• Migration filename prefix — both new migration files use xx_ rather than the hash-based prefix convention used elsewhere in this repo.
• Bare dict in SaaSConfigVersionDetailResponse — use Dict[str, Any] to match the sibling schema's style.
• Double None-coalescing — datasets if datasets else None at the call site combined with datasets or None inside create_snapshot is redundant; handle it in one place.

[claude]

Critical: Personal developer config should not be committed.

This file contains hardcoded paths to the author's local machine (/Users/bruno/Ethyca/...). It's a developer-specific MCP server configuration and should be added to .gitignore rather than committed to the repository. Anyone cloning the repo will have a broken/irrelevant .mcp.json pointing to non-existent paths on their machine.

[claude]

Unused imports.

load_config_from_string and load_dataset_from_string are imported here but never called in any of the new endpoint handlers in this file. They should be removed.

[claude]

Double redundancy: datasets if datasets else None → datasets or None → still None when list is empty.

The call site passes datasets if datasets else None and the create_snapshot body stores datasets or None. Both expressions reduce to the same thing — the double guard is confusing and implies the distinction matters somewhere in between. Pick one location for the None-coalescing and remove the other.

More importantly, an empty [] (no associated datasets found) and None (datasets intentionally not provided) have different semantics. If the intent is always to store None when there are no datasets, that's fine, but it should be explicit rather than implicit via two chained falsy checks.

[claude]

Model method doing cross-model querying and side-effect writes — consider moving to a service.

update_saas_config now imports DatasetConfig and ConnectionConfigSaaSHistory, queries related rows, and creates a history snapshot, all inside a model method. This makes the model aware of other models' internals and adds a hidden DB query on every call.

Ideally this orchestration belongs in a service layer that coordinates the update and the snapshot creation explicitly. At minimum, accepting a pre-fetched datasets list as an optional parameter to update_saas_config would keep the model's surface area narrow and let callers decide whether to include dataset context.

[claude]

Business logic directly in route handler.

Calling SaaSConfigVersion.upsert(...) here mixes persistence logic into the route layer. This belongs in a service (or at minimum, inside connection_config.update_saas_config()), so routes stay thin. The existing update_saas_config call just above already does the config update — the version upsert could be co-located there or delegated to a dedicated service method.

[claude]

Missing index on connection_key.

connection_key is explicitly denormalized so history remains queryable after the parent ConnectionConfig is deleted (when connection_config_id becomes NULL). But without an index on connection_key, those orphaned rows can only be found via a full table scan.

If the intent is purely for audit/ad-hoc use this may be acceptable, but since the docstring calls out post-deletion queryability as a design goal, an index would make that goal actionable:

op.create_index(
    op.f("ix_connection_config_saas_history_connection_key"),
    "connection_config_saas_history",
    ["connection_key"],
    unique=False,
)

[claude]

Use Dict[str, Any] instead of bare dict for consistency.

ConnectionConfigSaaSHistoryDetailResponse (the sibling schema) uses Dict[str, Any] for its config field. Using the bare dict annotation here is less explicit and inconsistent. Prefer Dict[str, Any] (already imported in the project's typing conventions) for both fields.

[claude]

No tests for the three new connector template version endpoints.

list_connector_template_versions, get_connector_template_version_config, and get_connector_template_version_dataset have no test coverage. The TestListSaaSConfigHistory / TestGetSaaSConfigHistoryByVersion classes cover the connection-scoped endpoints well — the same level of coverage (unauthenticated, wrong scope, not found, happy path) should exist for the template-version endpoints too.

[claude]

DB query helper living in the endpoint file.

_get_version_row performs a direct db.query(SaaSConfigVersion) inside connector_template_endpoints.py. Endpoint files in this codebase are generally thin — data access logic belongs in a service or repository layer. This helper (and the inline queries in list_connector_template_versions) should live in a service method.

[claude]

Startup queries one row per OOB connector on every boot.

sync_oob_saas_config_versions iterates over every bundled connector template and issues a SELECT per connector to check whether the row already exists. For OOB connectors (immutable once written) this is a no-op after first deploy, but the queries still fire on every startup.

Consider a set-difference approach: fetch all (connector_type, version) pairs already in the table in a single query and only parse + insert the ones that are missing. This keeps startup overhead bounded regardless of how many OOB connectors exist.