Skip to content

Conversation

@wadesdev
Copy link
Contributor

@wadesdev wadesdev commented Nov 26, 2025

Ticket ENG-2109

Description Of Changes

  • Adds read_only as a connection parameter to the MSSQL connection
    • This is expected to resolve a client issue wherein their database connections will require ApplicationIntent=ReadOnly (in ODBC parlance)

Code Changes

  • Updates front end to include an optional field in MSSQL configuration
  • Updates backend to include query parameters in the URL create function for building the MSSQL connection URI

Steps to Confirm

  1. Provision MSSQL server without "read only replica" option
  2. Create MSSQL connection in Fides with read only is True
  3. Confirm successful connection to MSSQL

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
Copy link

vercel bot commented Nov 26, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Dec 1, 2025 7:53pm
fides-privacy-center Ignored Ignored Dec 1, 2025 7:53pm

@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.00%. Comparing base (c3fc2c6) to head (d55d236).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7041      +/-   ##
==========================================
- Coverage   87.03%   87.00%   -0.03%     
==========================================
  Files         528      528              
  Lines       34668    34683      +15     
  Branches     4005     4010       +5     
==========================================
+ Hits        30174    30177       +3     
- Misses       3620     3630      +10     
- Partials      874      876       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wadesdev wadesdev marked this pull request as ready for review December 1, 2025 19:25
@wadesdev wadesdev requested review from a team as code owners December 1, 2025 19:25
@wadesdev wadesdev requested review from adamsachs and jpople and removed request for a team December 1, 2025 19:25
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 1, 2025

Greptile Overview

Greptile Summary

This PR adds a read_only_connection parameter to MSSQL connection configuration, allowing connections to specify read-only mode (e.g., ApplicationIntent=ReadOnly in ODBC parlance). The implementation adds the field to both backend schema and frontend TypeScript types, with appropriate defaults.

Key Changes:

  • Backend: Added read_only_connection boolean field to MicrosoftSQLServerSchema with default=False
  • Connector: Modified build_uri() to append read_only query parameter when enabled
  • Frontend: Added optional read_only_connection field to TypeScript schema
  • Tests: Updated integration tests and schema validation tests

Issues Found:

  • Query parameter format may be incorrect - SQLAlchemy URL.create() typically expects dict, not string
  • Parameter name read_only needs verification for pymssql/FreeTDS compatibility
  • Missing unit tests for URL building logic (other connectors have dedicated test files)
  • Test assertions don't use URL parsing to validate query parameters

Confidence Score: 3/5

  • This PR requires verification of query parameter format and pymssql compatibility before merging
  • The implementation has potential issues: (1) SQLAlchemy URL.create() query parameter format may be incorrect (string vs dict), (2) the parameter name 'read_only' needs verification for pymssql/FreeTDS compatibility, and (3) missing unit tests to validate URL construction. Integration tests pass but don't validate the actual URL format. These issues could cause runtime failures when the feature is actually used.
  • Pay close attention to src/fides/api/service/connectors/microsoft_sql_server_connector.py - verify query parameter format and parameter name

Important Files Changed

File Analysis

Filename Score Overview
src/fides/api/service/connectors/microsoft_sql_server_connector.py 3/5 Adds read_only query parameter to MSSQL connection URI - missing unit tests for URL building logic
src/fides/api/schemas/connection_configuration/connection_secrets_mssql.py 5/5 Adds read_only_connection boolean field to schema with appropriate defaults

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. src/fides/api/service/connectors/microsoft_sql_server_connector.py, line 25-48 (link)

    style: Missing unit tests for build_uri() method. Other connectors (PostgreSQL, MongoDB, MariaDB, MySQL) have dedicated test files in tests/ops/service/connection_config/ that test URL building logic. Consider creating test_mssql_connector.py to test:

    • URL with read_only_connection=True
    • URL with read_only_connection=False
    • URL with read_only_connection=None
    • Default behavior when field is not provided

    Also, tests should use URL parsing (per custom rule 49769419):

    from urllib.parse import urlparse, parse_qs
    parsed = urlparse(uri)
    query_params = parse_qs(parsed.query)
    assert query_params.get("read_only") == ["True"]

    Context Used: Rule from dashboard - When writing tests that validate URL properties, always parse the URL into components and assert on ... (source)

5 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@wadesdev wadesdev enabled auto-merge December 1, 2025 20:49
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for addressing the structural concern on the query args, and for providing some more evidence for why we think read_only should work here.

there's still some doubt about whether this is going to be effective, but i don't think that should hold us up from merging - let's see if we can figure out a way to validate it internally! at least our CI checks passing should make us feel better that we haven't broken anything core with the mssql integration 👍

@wadesdev wadesdev added this pull request to the merge queue Dec 2, 2025
Merged via the queue into main with commit 947023d Dec 2, 2025
75 checks passed
@wadesdev wadesdev deleted the wades/ENG-2109 branch December 2, 2025 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants