TT-17841: improved tests for persistent storage

[sredxny]

Description

Add a driver-agnostic conformance test suite for the persistent storage layer, and fix the driver bugs that suite uncovered (mostly PostgreSQL, plus a Mongo/mgo upsert-concurrency fix).

The persistent package supports multiple database drivers (mgo, official MongoDB, PostgreSQL) behind a single PersistentStorage interface, but there was no shared way to guarantee every driver behaves identically against that contract. Each driver had its own ad-hoc tests, so behavioral drift between MongoDB and PostgreSQL went undetected.

This PR introduces a contract-based conformance test suite (persistent/internal/testutil/suite.go) that runs the same behavioral assertions against every driver (conformance_mgo_test.go, conformance_mongo_test.go, conformance_postgres_test.go). Running the suite surfaced several correctness bugs, which are fixed here.

What we're doing

1. Conformance test framework

• Generic Suite + RunSuite harness validating any driver against the PersistentStorage interface (Ping, HasTable, Migrate/Drop, CRUD, Update, Upsert, query translation, indexes).
• Wired up for mgo, official Mongo, and Postgres so all three are held to the same contract.

2. PostgreSQL driver fixes

• Update never inserts a ghost row: Update now issues a single all-fields UPDATE via Select("*").Omit("id").Updates(object) instead of GORM's upsert-flavored Save. Because Updates never falls back to INSERT when the WHERE matches nothing, RowsAffected == 0 is a reliable signal that the record is missing (returning sql.ErrNoRows), and there is no TOCTOU window: a concurrent DELETE can no longer let the write resurrect the row, and an object with a zero/mismatched ID can no longer create a duplicate. This also removes the previous pre-COUNT + explicit transaction entirely.
• Concurrency-safe Upsert: Upsert acquires a pg_advisory_xact_lock (keyed on table + query) to serialize concurrent upserts of the same logical record and prevent duplicate inserts. Existence is determined via COUNT rather than RowsAffected, so an upsert with an empty update map no longer wrongly falls through to INSERT for an existing record. The transaction is managed with GORM's db.Transaction(...) (auto-rollback on error/panic, auto-commit otherwise). The advisory-lock key is derived by JSON-marshaling the sorted query values; a value that is not JSON-serializable is now rejected with an error rather than hashed via an ambiguous fmt.Sprintf fallback, so the key is always canonical (this closes a lock-key collision / false-contention vector flagged in review).
• $or query translation: multi-field conditions inside a single $or clause are now correctly grouped with AND in a nested sub-expression (previously they were flattened, producing incorrect boolean logic). Nested field names also get the same .→_ conversion and identifier sanitization as the non-$or path.
• TTL index detection: GetIndexes reads index_metadata to correctly flag IsTTLIndex and populate TTL. A missing index_metadata table (it is only created with the first TTL index) is treated as "no TTL metadata", but any other query error is surfaced instead of silently swallowed. The lookup is skipped entirely when the table has no secondary indexes to annotate, avoiding two needless round-trips on the common path.

3. Mongo / mgo driver fix

• Concurrency-safe Upsert: both the official Mongo and mgo drivers now retry (bounded) on a duplicate-key error from the upsert insert race. Servers before 5.0 do not retry the findAndModify(upsert:true) insert path internally, so concurrent upserts of the same not-yet-existing _id could return a transient E11000 to the caller; the losing call now re-reads the winner's document. This is required for the shared UpsertNoDuplicatesUnderConcurrency conformance assertion to hold on Mongo 4.2/4.4.

4. CI / build tooling

• CI passes a postgres_test_dsn matching the containerized Postgres credentials so the Postgres conformance tests run.
• Conformance and driver test files list build tags for both the literal and YAML-coerced matrix version strings (e.g. postgres16.10/postgres16.1, postgres15.0/postgres15, mongo7.0/mongo7, mongo6.0/mongo6) so the suites compile and run on every matrix row regardless of how the version token is rendered.
• Coverage uses -coverpkg across the storage tree and only iterates packages that actually have tests under the active build tag; empty-line coverage files (Go 1.25 + -coverpkg) are stripped before gocovmerge to prevent merge failures.
• SonarQube test inclusions extended to cover internal/testutil.

Related Issue

https://tyktech.atlassian.net/browse/TT-17841

Motivation and Context

There was no shared contract test guaranteeing consistent behavior across the persistent drivers, allowing behavioral drift (especially Postgres vs Mongo) to go unnoticed. Building the conformance suite exposed real correctness bugs around Update atomicity, Upsert concurrency (Postgres and Mongo), $or query logic, and TTL index reporting — all fixed here.

Acceptance Criteria

• A shared conformance suite exists and runs against all three persistent drivers (mgo, official Mongo, Postgres) in CI.
• Update on a non-existent record returns sql.ErrNoRows and never creates a new row.
• Update cannot produce a ghost insert under a concurrent delete (single atomic UPDATE that never falls back to INSERT).
• Concurrent Upsert calls for the same query do not create duplicate records (Postgres advisory lock; Mongo/mgo duplicate-key retry).
• Upsert with an empty update map on an existing record updates/returns that record rather than inserting a duplicate.
• A $or query with multiple fields per clause produces correct (a AND b) OR (c AND d) semantics, with proper field-name sanitization.
• GetIndexes correctly reports IsTTLIndex and TTL for TTL indexes, and does not error when no TTL metadata table exists.
• Coverage reports merge cleanly (no gocovmerge failures) and the new testutil code is included in Sonar analysis.
• make lint (gofumpt + golangci-lint) and the full test matrix pass.

Test Coverage For This Change

• Automated: task test-persistent DB=postgres DB_VERSION=16.10, DB=mongo DB_VERSION=7.0, and the mgo variant — all run the conformance suite (with -race).
• New unit tests: basic_operations_test.go, query_test.go covering the Update/Upsert/$or fixes.
• Concurrency: run with -race to confirm no data races and no duplicate rows under concurrent Update/Upsert.
• task merge-coverage produces a valid merged-coverage.cov.

Notes / Trade-offs

• Update is now a single all-fields UPDATE (no pre-COUNT, no explicit transaction). Upsert keeps one extra COUNT round-trip inside its advisory-lock transaction to correctly handle an empty update map.
• The pg_advisory_xact_lock in the Postgres Upsert serializes concurrent upserts that use the same (table, query) pair (intentional, for correctness). It does not guard upserts reaching the same row via a different filter, nor writers that bypass Upsert (Insert, raw SQL); uniqueness beyond the primary key is not enforced at the schema level.
• Advisory-lock keys are built by JSON-marshaling the sorted query values; non-JSON-serializable query values are rejected with an error so the key is always canonical.
• The Mongo/mgo upsert retry is bounded (3 attempts) and only triggers on duplicate-key errors.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring or add test (improvements in base code or adds test coverage to functionality)
• Documentation updates or improvements.

Checklist

• I have reviewed the guidelines for contributing to this repository.
• Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If PRing from your fork, don't come from your master!
• Make sure you are making a pull request against our master branch (left side). Also, it would be best if you started your change off our latest master.
• My change requires a change to the documentation.
  • I have manually updated the README(s)/documentation accordingly.
  • If you've changed APIs, describe what needs to be updated in the documentation.
• I have updated the documentation accordingly.
• Modules and vendor dependencies have been updated; run go mod tidy && go mod vendor
• When updating library version must provide reason/explanation for this update.
• I have added tests to cover my changes.
• All new and existing tests passed.
• Check your code additions will not fail linting checks:
  • gofmt -s -w .
  • go vet ./...

Ticket Details

TT-17841

Status	In Code Review
Summary	Persistent storage: add driver conformance test suite and fix PostgreSQL Update/Upsert/$or/TTL bugs

Generated at: 2026-08-14 03:52:26

[github-actions]

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA

---

1 out of 3 committers have signed the CLA.
✅ @sredxny
❌ @sredny buitrago
❌ @sredny Buitrago
sredny buitrago, Sredny Buitrago seem not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request}

[probelabs]

This pull request introduces a driver-agnostic conformance test suite for the persistent storage layer to ensure consistent behavior across different database backends. The new suite revealed several critical bugs in the PostgreSQL driver, which have been fixed, and also prompted improvements to the concurrency handling in the MongoDB drivers.

Files Changed Analysis

• New Files: The core of this PR is the new generic conformance test suite in persistent/internal/testutil/suite.go. New test files (persistent/conformance_mgo_test.go, persistent/conformance_mongo_test.go, persistent/conformance_postgres_test.go) have been added to execute this suite against each supported database driver.
• Modified Files: The majority of modifications are within the PostgreSQL driver (persistent/internal/driver/postgres/) to address bugs found by the new tests. Key changes are in basic_operations.go (Update/Upsert logic), query.go ($or translation), and indexes.go (TTL detection). The mgo and official mongo drivers (persistent/internal/driver/mgo/mgo.go, persistent/internal/driver/mongo/mongo.go) received targeted fixes for Upsert concurrency. Build and CI configurations in Taskfile.yml and .github/workflows/ci-tests.yml were updated to support the new test structure.
• Notable Patterns: The key pattern is the shift towards contract-based testing. A single, reusable test suite (testutil.RunSuite) now enforces consistent behavior across all supported databases, significantly improving the system's reliability and maintainability.

Architecture & Impact Assessment

• What this PR accomplishes:

  1. Standardizes Driver Testing: Implements a common test suite to verify that all persistent storage drivers adhere to the PersistentStorage interface contract.
  2. Fixes Critical PostgreSQL Bugs: Corrects data integrity bugs in the PostgreSQL driver related to Update atomicity, Upsert concurrency, $or query logic, and TTL index reporting.
  3. Improves MongoDB Concurrency: Makes Upsert operations in both MongoDB drivers safer under concurrent loads.

• Key technical changes introduced:

  1. Postgres Update Atomicity: The Update operation now uses a single atomic UPDATE statement (Updates instead of Save), preventing a race condition where a concurrent DELETE could lead to an unintended INSERT (a "ghost row").
  2. Postgres Upsert Concurrency: The Upsert operation now uses a transaction-scoped advisory lock (pg_advisory_xact_lock) to serialize concurrent attempts to insert the same record, preventing duplicate rows.
  3. Postgres $or Query Fix: The query translator now correctly groups multi-field conditions within an $or clause with AND in a nested sub-expression, fixing incorrect boolean logic.
  4. Mongo/mgo Upsert Retry: Both MongoDB drivers now retry Upsert operations on duplicate-key errors, handling a race condition on servers older than v5.0.

• Affected system components:

  • The entire persistent storage layer, particularly the PostgreSQL driver.
  • Any application service relying on the storage layer will benefit from improved data integrity, query correctness, and reliability, especially under high-concurrency workloads.

• Component Relationships:

graph TD
subgraph "Test Framework"
A["Conformance Test Suite
(testutil.RunSuite)"] --> B{PersistentStorage Interface}
end

subgraph "Database Drivers"
    C[MgoDriver] -- implements --> B
    D[MongoDriver] -- implements --> B
    E[PostgresDriver] -- implements --> B
end

A -- validates --> C
A -- validates --> D
A -- validates --> E

style E fill:#f8d7da,stroke:#721c24,stroke-width:2px
style A fill:#d4edda,stroke:#155724

## Scope Discovery & Context Expansion
- The introduction of the conformance suite is a foundational improvement for the entire data persistence layer. While the immediate code changes are confined to the `persistent` module, the fixes have broader implications for system stability and data integrity.
- **Data Integrity**: The `Upsert` concurrency fixes are critical for preventing data duplication in high-throughput services. The `Update` atomicity fix prevents silent data corruption.
- **Query Correctness**: The fix for the `$or` operator ensures that complex queries behave as expected, preventing subtle bugs in application logic that could lead to incorrect data retrieval or updates.
- **Maintainability**: Future development of new storage drivers is now significantly de-risked. Developers can implement the `PersistentStorage` interface and validate their implementation against the conformance suite to ensure it meets the required behavioral contract from the outset.


<details>
  <summary>Metadata</summary>

  - Review Effort: 4 / 5
  - Primary Label: enhancement


</details>
<!-- visor:section-end id="overview" -->

<!-- visor:thread-end key="TykTechnologies/storage#158@98a2e0b" -->

---

*Powered by [Visor](https://probelabs.com/visor) from [Probelabs](https://probelabs.com)*

*Last updated: 2026-08-14T03:53:30.531Z | Triggered by: pr_updated | Commit: 98a2e0b*

💡 **TIP:** You can chat with Visor using `/visor ask <your question>`
<!-- /visor-comment-id:visor-thread-overview-TykTechnologies/storage#158 -->

[probelabs]

\n\n

✅ Architecture Check Passed

No architecture issues found – changes LGTM.

Performance Issues (1)

Severity	Location	Issue
🟡 Warning	persistent/internal/driver/postgres/basic_operations.go:547-551	The `upsertLockKey` function uses `json.Marshal` to serialize query values for generating an advisory lock key. This is executed for every `Upsert` call. While JSON marshaling provides a canonical representation which is important for correctness, it can be resource-intensive (CPU and memory) for large or complex query values, potentially creating a performance bottleneck under high `Upsert` load. 💡 Suggestion For performance-critical paths, consider if a more lightweight serialization method could be used. If query values are known to be simple types, a type switch could handle serialization more efficiently than reflection-based `json.Marshal`. Alternatively, document that `Upsert` query values should be kept simple to avoid performance degradation.

✅ Quality Check Passed

No quality issues found – changes LGTM.

---

Powered by Visor from Probelabs

Last updated: 2026-08-14T03:53:27.250Z | Triggered by: pr_updated | Commit: 98a2e0b

💡 TIP: You can chat with Visor using /visor ask <your question>

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud