improved tests for persistent storage

[sredxny]

Description

Related Issue

Motivation and Context

Test Coverage For This Change

Screenshots (if appropriate)

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 ./...

[github-actions]

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you 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

---

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]

This pull request introduces a comprehensive conformance test suite for the persistent storage layer to ensure consistent behavior across all database drivers. The implementation of this suite revealed and fixed several critical bugs in the PostgreSQL driver, particularly around transaction atomicity, concurrency, and query logic.

Files Changed Analysis

• Added Files: The core of this PR is the new generic conformance test suite in persistent/internal/testutil/suite.go. To execute this, new test files (persistent/conformance_mgo_test.go, persistent/conformance_mongo_test.go, persistent/conformance_postgres_test.go) have been added to run the suite against each 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.
• 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 (mongo, mgo, postgres) 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 transaction and includes a pre-check to verify record existence, ensuring it correctly returns sql.ErrNoRows instead of performing 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.
  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, 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.
- **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.


<details>
<summary>Metadata</summary>

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


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

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

---

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

*Last updated: 2026-07-21T19:57:50.541Z | Triggered by: pr_updated | Commit: caa497b*

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

[probelabs]

Security Issues (3)

Severity	Location	Issue
🟡 Warning	.github/workflows/ci-tests.yml:191	The Postgres test DSN contains hardcoded credentials (`user=testuser password=testpass`). While this is for a test environment, it's a security risk to store credentials directly in version control. They could be exposed in build logs or if the repository's visibility changes. 💡 Suggestion Store the test database credentials in GitHub secrets and reference them in the workflow. This prevents them from being stored in plaintext in the repository.
🟡 Warning	persistent/conformance_postgres_test.go:30	A fallback database connection string with hardcoded credentials (`user=testuser password=testpass`) is present in the source code. Storing credentials in code is a security risk. This DSN is used if the `postgres_test_dsn` environment variable is not set, making it easy to accidentally use these credentials. 💡 Suggestion Remove the hardcoded fallback DSN. Instead, require the `postgres_test_dsn` environment variable to be set for the test to run, and skip the test if it's missing. This enforces a secure practice of managing credentials outside of the source code.
🟡 Warning	persistent/conformance_postgres_test.go:30	The fallback database connection string for PostgreSQL tests explicitly disables SSL (`sslmode=disable`). This means data transmitted between the test runner and the database is unencrypted. While intended for local testing, this sets an insecure precedent and could be problematic if tests are ever run against a non-local database. 💡 Suggestion Change `sslmode=disable` to `sslmode=prefer` or `sslmode=require` in the connection string. For local testing where the server may not have SSL, `prefer` is a reasonable default. This encourages secure connections where available.

Security Issues (3)

Severity	Location	Issue
🟡 Warning	.github/workflows/ci-tests.yml:191	The Postgres test DSN contains hardcoded credentials (`user=testuser password=testpass`). While this is for a test environment, it's a security risk to store credentials directly in version control. They could be exposed in build logs or if the repository's visibility changes. 💡 Suggestion Store the test database credentials in GitHub secrets and reference them in the workflow. This prevents them from being stored in plaintext in the repository.
🟡 Warning	persistent/conformance_postgres_test.go:30	A fallback database connection string with hardcoded credentials (`user=testuser password=testpass`) is present in the source code. Storing credentials in code is a security risk. This DSN is used if the `postgres_test_dsn` environment variable is not set, making it easy to accidentally use these credentials. 💡 Suggestion Remove the hardcoded fallback DSN. Instead, require the `postgres_test_dsn` environment variable to be set for the test to run, and skip the test if it's missing. This enforces a secure practice of managing credentials outside of the source code.
🟡 Warning	persistent/conformance_postgres_test.go:30	The fallback database connection string for PostgreSQL tests explicitly disables SSL (`sslmode=disable`). This means data transmitted between the test runner and the database is unencrypted. While intended for local testing, this sets an insecure precedent and could be problematic if tests are ever run against a non-local database. 💡 Suggestion Change `sslmode=disable` to `sslmode=prefer` or `sslmode=require` in the connection string. For local testing where the server may not have SSL, `prefer` is a reasonable default. This encourages secure connections where available.

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

Severity	Location	Issue
🟡 Warning	persistent/internal/driver/postgres/basic_operations.go:546-549	The `upsertLockKey` function uses `fmt.Sprintf("%v", ...)` as a fallback for values that cannot be marshaled to JSON. This can create unstable lock keys if the query contains complex types like pointers or structs without a stable string representation, as their formatted value (e.g., memory address) is not consistent. This could lead to different goroutines acquiring locks for what should be the same logical operation, defeating the purpose of the advisory lock and potentially re-introducing race conditions. 💡 Suggestion Instead of falling back to a potentially unstable representation, consider returning an error if a value in the upsert query is not JSON-serializable. This would enforce a stricter contract on the caller, ensuring that only values with a stable, canonical representation are used for locking, which makes the concurrency control more robust.

Performance Issues (3)

Severity	Location	Issue
🟡 Warning	persistent/internal/driver/postgres/basic_operations.go:149-154	The `Update` function now includes an additional `COUNT` query before the `Save` operation. This adds an extra database round-trip to every update call, increasing latency. While this is a deliberate trade-off to fix a time-of-check to time-of-use (TOCTOU) bug and ensure `sql.ErrNoRows` is returned correctly, it has a performance cost. 💡 Suggestion The current implementation is a valid trade-off for correctness. For potential future optimization, consider using a single `UPDATE ... RETURNING id` statement and checking if a value was returned. This would combine the existence check and the update into a single database operation, but may be complex to integrate with GORM's generic `Save` method.
🟡 Warning	persistent/internal/driver/postgres/basic_operations.go:363-385	The `Upsert` function now uses a `pg_advisory_xact_lock` and performs an extra `COUNT` query. The advisory lock serializes concurrent `Upsert` operations on the same logical record, which is necessary for correctness but can become a bottleneck under high contention. The additional `COUNT` query adds another database round-trip within the lock. These changes increase latency and reduce throughput for `Upsert` operations, particularly for contended records. 💡 Suggestion The use of an advisory lock is a standard and correct pattern to prevent race conditions in `Upsert` logic. The performance impact is an inherent trade-off for data integrity. No code change is recommended, but developers should be aware of the new performance characteristics of this operation.
🟡 Warning	persistent/internal/driver/postgres/basic_operations.go:536-542	The `upsertLockKey` function uses `json.Marshal` to serialize query values for generating an advisory lock key. JSON marshaling relies on reflection and can be computationally expensive, especially for complex data structures. As this function is on the hot path for all `Upsert` operations, this could introduce a performance bottleneck if query values are not simple primitives. 💡 Suggestion To optimize for common cases, consider using a type switch to handle primitive types (e.g., strings, integers, byte slices) with more direct and performant serialization methods, falling back to `json.Marshal` only for complex or unknown types. This would reduce the overhead for the majority of `Upsert` queries.

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-07-21T19:57:19.444Z | Triggered by: pr_updated | Commit: caa497b

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

[sonarqubecloud]

Quality Gate failed

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

See analysis details on SonarQube Cloud

💡 Need a hand with PR review? Try Gitar by Sonar!

[github-actions]

🚨 Jira Linter Failed

Commit: caa497b
Failed at: 2026-07-21 19:56:10 UTC

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

🔍 Click to view error details

failed to validate branch and PR title rules: neither branch name 'improve-tests-fix-postgres-transactions' nor PR title 'improved tests for persistent storage' contains a valid Jira ticket ID (e.g., ABC-123)

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.