TT-17841: improved tests for persistent storage by sredxny · Pull Request #158 · TykTechnologies/storage

sredxny · 2026-07-21T03:05:55Z

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

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-10 22:03:58

github-actions · 2026-07-21T03:06:08Z

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 · 2026-07-21T03:06:44Z

This pull request introduces a driver-agnostic conformance test suite for the persistent storage layer to ensure consistent behavior across different database implementations. The new test suite uncovered several critical correctness bugs in the PostgreSQL driver, which have been fixed. The changes include making Update operations atomic, Upsert operations concurrency-safe, correcting $or query translation logic, and improving TTL index detection.

Files Changed Analysis

Added 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/). These changes address issues with transaction handling in Update, race conditions in Upsert, logical errors in $or query translation, and incorrect TTL index detection. Build and CI configurations in Taskfile.yml and .github/workflows/ci-tests.yml are also updated to support the new test structure and fix coverage reporting.
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 (mgo, official MongoDB, PostgreSQL) adhere to the PersistentStorage interface contract.
2. Fixes Critical PostgreSQL Bugs: Corrects bugs in the PostgreSQL driver related to transaction atomicity, concurrency race conditions, and query logic, improving data integrity.
Key technical changes introduced:
1. Atomic Updates in Postgres: The Update operation now uses tx.Select("*").Omit("id").Updates(object) within a transaction to ensure atomicity and prevent a TOCTOU race condition where a concurrent delete could cause an unintended insert.
2. Concurrency Control for Postgres Upsert: The Upsert operation now uses pg_advisory_xact_lock to prevent race conditions where concurrent calls could create duplicate records.
3. Correct $or Query Translation: The logic for handling $or queries in Postgres has been fixed. Multi-field conditions within an $or element are now correctly grouped with AND in a nested sub-expression.
4. Improved TTL Index Detection: The GetIndexes function for Postgres now correctly identifies TTL indexes by querying the index_metadata table and handles cases where the table does not exist.
Affected system components:
- The persistent storage layer, with a focus on the PostgreSQL driver implementation.
- Any application service relying on the PostgreSQL driver will benefit from improved data integrity and query correctness, especially under high-concurrency workloads.
Component Relationships:

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

subgraph "Storage 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.
- **Data Integrity**: The `Upsert` concurrency fix is critical for preventing data duplication in high-throughput services. The `Update` atomicity fix prevents silent failures and unintended data creation.
- **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@c2fb84c" -->

---

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

*Last updated: 2026-08-10T22:01:09.062Z | Triggered by: pr_updated | Commit: c2fb84c*

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

probelabs · 2026-07-21T03:07:15Z

Security Issues (2)

Severity	Location	Issue
🟠 Error	`persistent/internal/driver/postgres/query.go:298`	The value `val` is directly embedded into the SQL query using `fmt.Sprint(nv)`, which can lead to SQL injection if `nv` contains malicious input. Although GORM's `Where` clause with `?` placeholders is used, the value is being prepared outside of it, which is risky. GORM should handle the value conversion. 💡 Suggestion Pass the value `nv` directly to GORM's `Where` clause instead of converting it to a string with `fmt.Sprint`. This allows the database driver to handle proper parameterization and prevent SQL injection vulnerabilities. Change `sub = sub.Where(col+" = ?", val)` to `sub = sub.Where(col+" = ?", nv)`. 🔧 Suggested Fix `sub = sub.Where(col+" = ?", nv)`
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:528`	The `upsertLockKey` function uses `json.Marshal` on query values to generate a lock key. If a query value contains a complex or deeply nested structure, this marshaling process could consume significant CPU and memory, potentially leading to a denial-of-service vulnerability if the query parameters can be influenced by an external user. 💡 Suggestion Consider adding a check to limit the size or depth of the query values before marshaling. Alternatively, if the values are not complex, document the expectation that only simple, serializable values should be used in queries for `Upsert` operations.

Performance Issues (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:547`	The `upsertLockKey` function uses `json.Marshal` to serialize query values for generating an advisory lock key. While this ensures a canonical representation, `json.Marshal` uses reflection and can be slow for large or complex data structures, adding CPU and memory overhead to every `Upsert` call in the PostgreSQL driver. This could become a bottleneck under high load with complex query filters. 💡 Suggestion For better performance, consider replacing `json.Marshal` with a more efficient, type-aware serialization mechanism that avoids reflection. A type switch on common query value types (strings, numbers, `model.ObjectID`) with a fallback to a more performant serialization library could reduce overhead. If complex types are not expected in query values, a simple type-switch would be much faster.

Security Issues (2)

Severity	Location	Issue
🟠 Error	`persistent/internal/driver/postgres/query.go:298`	The value `val` is directly embedded into the SQL query using `fmt.Sprint(nv)`, which can lead to SQL injection if `nv` contains malicious input. Although GORM's `Where` clause with `?` placeholders is used, the value is being prepared outside of it, which is risky. GORM should handle the value conversion. 💡 Suggestion Pass the value `nv` directly to GORM's `Where` clause instead of converting it to a string with `fmt.Sprint`. This allows the database driver to handle proper parameterization and prevent SQL injection vulnerabilities. Change `sub = sub.Where(col+" = ?", val)` to `sub = sub.Where(col+" = ?", nv)`. 🔧 Suggested Fix `sub = sub.Where(col+" = ?", nv)`
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:528`	The `upsertLockKey` function uses `json.Marshal` on query values to generate a lock key. If a query value contains a complex or deeply nested structure, this marshaling process could consume significant CPU and memory, potentially leading to a denial-of-service vulnerability if the query parameters can be influenced by an external user. 💡 Suggestion Consider adding a check to limit the size or depth of the query values before marshaling. Alternatively, if the values are not complex, document the expectation that only simple, serializable values should be used in queries for `Upsert` operations.

\n\n \n\n

Performance Issues (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:547`	The `upsertLockKey` function uses `json.Marshal` to serialize query values for generating an advisory lock key. While this ensures a canonical representation, `json.Marshal` uses reflection and can be slow for large or complex data structures, adding CPU and memory overhead to every `Upsert` call in the PostgreSQL driver. This could become a bottleneck under high load with complex query filters. 💡 Suggestion For better performance, consider replacing `json.Marshal` with a more efficient, type-aware serialization mechanism that avoids reflection. A type switch on common query value types (strings, numbers, `model.ObjectID`) with a fallback to a more performant serialization library could reduce overhead. If complex types are not expected in query values, a simple type-switch would be much faster.

\n\n ### Quality Issues (1)

Severity	Location	Issue
🟠 Error	`persistent/internal/driver/postgres/basic_operations.go:561`	The `upsertLockKey` function uses `json.Marshal` to create a canonical representation of query values for hashing. However, `json.Marshal` does not guarantee a stable key order for map types (`map[string]interface{}` or `model.DBM`). If a query value is a map, concurrent callers could build logically equivalent queries that serialize differently, resulting in different lock keys. This would defeat the advisory lock and reintroduce the race condition that allows duplicate records to be inserted during concurrent upserts. 💡 Suggestion To ensure the advisory lock is consistently applied for logically identical queries, the hash generation must be based on a canonical representation of the query. Replace the standard `json.Marshal` with a method that guarantees deterministic output for maps, such as by using a canonical JSON library (e.g., `github.com/gibson042/canonicaljson-go`) or by implementing a recursive function to sort all nested maps before serialization.

Powered by Visor from Probelabs

Last updated: 2026-08-10T22:01:04.662Z | Triggered by: pr_updated | Commit: c2fb84c

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

…gainst a real Postgres 16.10 under the postgres16.10 tag; Mongo conformance passes under mongo7.0. Not touched: the pre-existing gofumpt nit in storage.go (outside these findings).

Address Visor review: upsertLockKey's fmt.Sprintf("%v") fallback for non-JSON-serializable query values is not canonical, so distinct queries could collide on the advisory-lock key (false contention / DoS on attacker-controlled input) or the same query could hash differently (missed lock, reintroducing the upsert race). Return an error instead of the ambiguous fallback and propagate it from Upsert, making the lock key fully deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…okup Apply /simplify cleanups from PR review: - Upsert: replace hand-rolled tx.Begin()/recover-defer/8x Rollback/Commit with d.db.Transaction(func(tx) error {...}), which auto-rolls-back on any returned error or panic and auto-commits otherwise. Behavior-preserving (advisory lock is transaction-scoped either way); removes the maintenance hazard of a forgotten Rollback on a future edit. - GetIndexes: skip the index_metadata existence check and TTL query when there are no secondary indexes to annotate, avoiding two DB round-trips on the common no-secondary-index path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

sonarqubecloud · 2026-08-10T22:03:17Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

improved tests for persistent storage

dddc216

sredny buitrago added 6 commits July 20, 2026 23:13

create blackbox tests for persistent

80abf4e

improve test coverage

e377282

on update if record not found return not found err

bd3850f

when updating if record not found return sql.ErrNoRows

587873b

addressing visor comments

1108e02

address review comments

caa497b

sredxny changed the title ~~improved tests for persistent storage~~ TT-17841: improved tests for persistent storage Jul 29, 2026

sredxny and others added 5 commits August 5, 2026 22:19

Merge branch 'main' into improve-tests-fix-postgres-transactions

a5ccf30

Postgres conformance suite + driver Update/Upsert//index tests pass a…

7455951

…gainst a real Postgres 16.10 under the postgres16.10 tag; Mongo conformance passes under mongo7.0. Not touched: the pre-existing gofumpt nit in storage.go (outside these findings).

Merge branch 'main' into improve-tests-fix-postgres-transactions

c2fb84c

Navigation Menu

Uh oh!

TT-17841: improved tests for persistent storage - #158

TT-17841: improved tests for persistent storage#158
sredxny wants to merge 12 commits into
mainfrom
improve-tests-fix-postgres-transactions

sredxny commented Jul 21, 2026 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

probelabs Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

probelabs Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

sonarqubecloud Bot commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

sredxny commented Jul 21, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What we're doing

Related Issue

Motivation and Context

Acceptance Criteria

Test Coverage For This Change

Notes / Trade-offs

Types of changes

Checklist

Ticket Details

Uh oh!

github-actions Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

probelabs Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files Changed Analysis

Architecture & Impact Assessment

Uh oh!

probelabs Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Issues (2)

Performance Issues (1)

Security Issues (2)

Performance Issues (1)

Uh oh!

sonarqubecloud Bot commented Aug 10, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sredxny commented Jul 21, 2026 •

edited by github-actions Bot

Loading

github-actions Bot commented Jul 21, 2026 •

edited

Loading

probelabs Bot commented Jul 21, 2026 •

edited

Loading

probelabs Bot commented Jul 21, 2026 •

edited

Loading