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 PostgreSQL driver bugs that suite uncovered.

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 against PostgreSQL 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

Atomic Update: Update now runs inside a real DB transaction. An existence pre-check (COUNT) plus Save are made atomic to avoid a TOCTOU race where a concurrent DELETE could let GORM's upsert-flavored Save INSERT a "ghost" row instead of returning sql.ErrNoRows. A missing record now correctly returns sql.ErrNoRows.
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 now 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.
$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 now reads index_metadata to correctly flag IsTTLIndex and populate TTL, tolerating the case where the metadata table doesn't yet exist.

3. CI / build tooling

CI passes a postgres_test_dsn matching the containerized Postgres credentials so the Postgres conformance tests run.
Coverage now 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 in the PostgreSQL driver around Update atomicity, Upsert concurrency, $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 is atomic: a concurrent delete between the existence check and the save cannot produce a ghost insert.
Concurrent Upsert calls for the same query do not create duplicate records.
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 and Upsert each add one extra COUNT round-trip for correctness.
The pg_advisory_xact_lock in Upsert serializes concurrent upserts of the same key (intentional, for correctness).
Lock keys are built by JSON-marshaling query values with an fmt.Sprintf fallback for non-serializable values.

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	Open
Summary	Persistent storage: add driver conformance test suite and fix PostgreSQL Update/Upsert/$or/TTL bugs

Generated at: 2026-08-06 02:19:57

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 2 committers have signed the CLA.
✅ @sredxny
❌ @sredny buitrago
sredny buitrago seems 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 is now wrapped in a database transaction and includes a pre-check to verify record existence, ensuring it correctly returns sql.ErrNoRows instead of performing an unintended insert on a miss.
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.
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@a5ccf30" -->

---

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

*Last updated: 2026-08-06T02:21:56.626Z | Triggered by: pr_updated | Commit: a5ccf30*

💡 **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 (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:539-542`	The fallback to `fmt.Sprintf("%v", ...)` for non-JSON-serializable values in `upsertLockKey` can produce non-deterministic lock keys. If a value is a pointer or a struct whose string representation includes a memory address, two logically identical queries could generate different lock keys. This would defeat the purpose of the `pg_advisory_xact_lock`, potentially re-introducing the race condition that allows duplicate records to be created under concurrent `Upsert` calls. 💡 Suggestion To ensure deterministic lock keys, either return an error or panic if a query value cannot be marshaled to JSON. This enforces a stricter contract on `Upsert` callers, preventing the use of values that cannot be reliably serialized. Panicking is a reasonable approach if non-serializable values are considered a programming error. Example fix (panicking): b, err := json.Marshal(query[k]) if err != nil { // A non-serializable value indicates a programming error. The caller // of Upsert should provide a query that can be deterministically // represented. A non-deterministic key would break the lock. panic(fmt.Sprintf("upsertLockKey: query value for key %q is not JSON-serializable: %v", k, err)) }

Architecture Issues (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:584`	The fallback to `fmt.Sprintf` for non-JSON-serializable values in `upsertLockKey` could be non-deterministic for certain complex data types, such as structs containing maps or pointers. If a query value is of such a type and also fails to serialize to JSON, `fmt.Sprintf("%v", ...)` does not guarantee a stable representation (e.g., map key order is not defined). This could result in different lock keys being generated for logically identical queries, potentially negating the concurrency protection of the advisory lock in rare edge cases. 💡 Suggestion For a more robust solution, consider implementing a canonical serialization fallback that guarantees determinism for a wider range of types. Alternatively, document the types of values that are not safely supported in `Upsert` queries. Given that query values are typically simple primitives, the practical risk is low, but improving determinism would make the locking mechanism more resilient.

Security Issues (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:539-542`	The fallback to `fmt.Sprintf("%v", ...)` for non-JSON-serializable values in `upsertLockKey` can produce non-deterministic lock keys. If a value is a pointer or a struct whose string representation includes a memory address, two logically identical queries could generate different lock keys. This would defeat the purpose of the `pg_advisory_xact_lock`, potentially re-introducing the race condition that allows duplicate records to be created under concurrent `Upsert` calls. 💡 Suggestion To ensure deterministic lock keys, either return an error or panic if a query value cannot be marshaled to JSON. This enforces a stricter contract on `Upsert` callers, preventing the use of values that cannot be reliably serialized. Panicking is a reasonable approach if non-serializable values are considered a programming error. Example fix (panicking): b, err := json.Marshal(query[k]) if err != nil { // A non-serializable value indicates a programming error. The caller // of Upsert should provide a query that can be deterministically // represented. A non-deterministic key would break the lock. panic(fmt.Sprintf("upsertLockKey: query value for key %q is not JSON-serializable: %v", k, err)) }

\n\n ### Architecture Issues (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:584`	The fallback to `fmt.Sprintf` for non-JSON-serializable values in `upsertLockKey` could be non-deterministic for certain complex data types, such as structs containing maps or pointers. If a query value is of such a type and also fails to serialize to JSON, `fmt.Sprintf("%v", ...)` does not guarantee a stable representation (e.g., map key order is not defined). This could result in different lock keys being generated for logically identical queries, potentially negating the concurrency protection of the advisory lock in rare edge cases. 💡 Suggestion For a more robust solution, consider implementing a canonical serialization fallback that guarantees determinism for a wider range of types. Alternatively, document the types of values that are not safely supported in `Upsert` queries. Given that query values are typically simple primitives, the practical risk is low, but improving determinism would make the locking mechanism more resilient.

\n\n ### Performance Issues (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:539`	The `upsertLockKey` function uses `json.Marshal` inside a loop to serialize query values for generating a lock key. `json.Marshal` relies on reflection and can be CPU-intensive, potentially creating a bottleneck in high-throughput scenarios where `Upsert` is called frequently. The fallback to `fmt.Sprintf` also adds overhead. 💡 Suggestion For better performance, consider replacing `json.Marshal` with a more direct serialization approach that avoids reflection for common data types. A type switch on the value to handle primitives (strings, numbers, ObjectIDs) directly and write their byte representations to the hash would be more efficient. The current reflection-based approach could be kept as a fallback for complex or unknown types.

Quality Issues (1)

Severity	Location	Issue
🟡 Warning	`persistent/internal/driver/postgres/basic_operations.go:539-540`	The fallback to `fmt.Sprintf` for non-JSON-serializable values in `upsertLockKey` could be unstable. The `%v` format for complex types is not guaranteed to be unique or consistent, which could lead to different lock keys for semantically identical queries. This might result in failed locking and potential race conditions. 💡 Suggestion To ensure lock key stability, it's safer to return an error if a query value cannot be marshaled to JSON. This enforces that only serializable, and thus canonicalizable, types are used in upsert queries, making the lock key generation fully deterministic. If supporting arbitrary types is necessary, a more robust serialization method than `fmt.Sprintf` should be used.

Powered by Visor from Probelabs

Last updated: 2026-08-06T02:21:06.184Z | Triggered by: pr_updated | Commit: a5ccf30

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

github-actions · 2026-08-06T02:20:00Z

🚨 Jira Linter Failed

Commit: a5ccf30
Failed at: 2026-08-06 02:19:58 UTC

The Jira linter failed to validate your PR. Please check the error details below:

🔍 Click to view error details

failed to validate Jira issue: jira ticket TT-17841 has status 'Open' but must be one of: Dod Check, Merge, In Design Review, In Dev, In Code Review, Ready For Dev

Next Steps

Ensure your branch name contains a valid Jira ticket ID (e.g., ABC-123)
Verify your PR title matches the branch's Jira ticket ID
Check that the Jira ticket exists and is accessible

This comment will be automatically deleted once the linter passes.

sonarqubecloud · 2026-08-06T02:24:02Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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

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

a5ccf30

Navigation Menu

Uh oh!

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

TT-17841: improved tests for persistent storage#158
sredxny wants to merge 8 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!

github-actions Bot commented Aug 6, 2026

Uh oh!

sonarqubecloud Bot commented Aug 6, 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 (1)

Architecture Issues (1)

Security Issues (1)

Quality Issues (1)

Uh oh!

github-actions Bot commented Aug 6, 2026

🚨 Jira Linter Failed

Next Steps

Uh oh!

sonarqubecloud Bot commented Aug 6, 2026

Quality Gate failed

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