Skip to content

fix: TT-16778 GORM v2 table sharding bug#948

Closed
probelabs[bot] wants to merge 1 commit intomasterfrom
fix/gorm-v2-sharding-bug
Closed

fix: TT-16778 GORM v2 table sharding bug#948
probelabs[bot] wants to merge 1 commit intomasterfrom
fix/gorm-v2-sharding-bug

Conversation

@probelabs
Copy link
Copy Markdown
Contributor

@probelabs probelabs bot commented Mar 10, 2026

Problem / Task

In SQLAggregatePump, SQLPump, and GraphSQLPump, the .WithContext(ctx) call drops the previously applied .Table(table) modifier. This causes the INSERT statement to target the default table instead of the sharded table.

Changes

The fix is to chain the .Table(table) modifier after .WithContext(ctx) so that the INSERT statement correctly targets the sharded table.

Testing

Added unit tests to verify the fix.


Requested by: @U3P2L4XNE | Trace: 04efb72f02ec22605b4e0aba22e102f6
Generated with Tyk AI Assistant

@probelabs
Copy link
Copy Markdown
Contributor Author

probelabs bot commented Mar 10, 2026

This PR fixes a critical bug in the SQL-based data pumps where GORM v2's method chaining order caused analytics data to be written to the default table instead of the correct date-sharded table.

Files Changed Analysis

  • Total Files: 7 files were changed (3 modified, 2 added, 2 dependency files).
  • Core Logic: The fix is consistently applied to pumps/graph_sql.go, pumps/sql.go, and pumps/sql_aggregate.go.
  • Testing: A significant portion of the change (+143 lines) is dedicated to new unit tests (pumps/sql_aggregate_sharding_sqlite_test.go, pumps/sql_aggregate_sharding_test.go), using both in-memory SQLite and go-sqlmock to validate the fix.
  • Dependencies: go.mod and go.sum were updated to include go-sqlmock for the new tests.

Architecture & Impact Assessment

What this PR accomplishes:
This PR corrects the data insertion logic for SQL pumps when table sharding is enabled. Previously, the .WithContext(ctx) call would reset the GORM query builder, causing the .Table(sharded_table) modifier to be ignored. This resulted in all data being inserted into the main, non-sharded table, breaking the sharding functionality.

Key technical changes introduced:
The fix involves changing the GORM method chain order to call .Table(tableName) after .WithContext(ctx). This ensures the sharded table is correctly targeted for the Create operation. The change also refactors table creation logic to use a locally-scoped db variable instead of mutating the pump's shared DB instance, improving safety.

sequenceDiagram
    participant P as Pump
    participant G as GORM
    participant DB as Database

    Note over P, G: Before Fix
    P->>G: .Table("sharded_table")
    P->>G: .WithContext(ctx)
    P->>G: .Create(data)
    G-->>DB: INSERT INTO "default_table" ...
    Note right of DB: Incorrect: .Table() was ignored

    Note over P, G: After Fix
    P->>G: .WithContext(ctx)
    P->>G: .Table("sharded_table")
    P->>G: .Create(data)
    G-->>DB: INSERT INTO "sharded_table" ...
    Note right of DB: Correct
Loading

Affected system components:

  • SQLPump
  • SQLAggregatePump
  • GraphSQLPump
  • Any user of tyk-pump relying on the table_sharding feature with a SQL backend (e.g., PostgreSQL, MySQL, SQLite).

Scope Discovery & Context Expansion

The bug's impact is localized to the tyk-pump service but affects a core data persistence feature. The fix is applied to all three SQL-based pump implementations, demonstrating a consistent correction of a faulty pattern. The configuration for this feature is in pump.example.conf under the table_sharding flag. The WriteData method of the Pump interface (defined in pumps/pump.go) is the entry point for the logic being fixed.

Metadata
  • Review Effort: 2 / 5
  • Primary Label: bug

Powered by Visor from Probelabs

Last updated: 2026-03-10T06:51:51.645Z | Triggered by: pr_opened | Commit: e39bc8d

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

@probelabs
Copy link
Copy Markdown
Contributor Author

probelabs bot commented Mar 10, 2026

Architecture Issues (1)

Severity Location Issue
🟠 Error pumps/sql.go:470-473
The refactoring of `ensureTable` to no longer modify the pump's `c.db` instance is a positive architectural change that reduces side effects. However, this change was incomplete. The non-sharded write path in `WriteData` (around line 281) still relies on the previous behavior where `c.db` was pre-configured with the correct table name during pump initialization. Since `ensureTable` no longer sets the table name on `c.db`, non-sharded writes will now target the wrong table (the GORM default `analytics_records` instead of `tyk_analytics`) or fail.
💡 SuggestionTo fix this regression, the non-sharded write operation in `WriteData` should be updated to explicitly specify the target table, making it independent of the state of `c.db`. This will make its behavior consistent with the sharded write path.
// in pumps/sql.go, inside WriteData()
if !c.SQLConf.TableSharding {
	recs := make([]*analytics.AnalyticsRecord, 0, dataLen)
	for _, r := range data {
		recs = append(recs, r.(*analytics.AnalyticsRecord))
	}
	tx := c.db.WithContext(ctx).Table(analytics.SQLTable).Create(recs)
	if tx.Error != nil {
		c.log.Error(tx.Error)
	}
}
\n\n

Architecture Issues (1)

Severity Location Issue
🟠 Error pumps/sql.go:470-473
The refactoring of `ensureTable` to no longer modify the pump's `c.db` instance is a positive architectural change that reduces side effects. However, this change was incomplete. The non-sharded write path in `WriteData` (around line 281) still relies on the previous behavior where `c.db` was pre-configured with the correct table name during pump initialization. Since `ensureTable` no longer sets the table name on `c.db`, non-sharded writes will now target the wrong table (the GORM default `analytics_records` instead of `tyk_analytics`) or fail.
💡 SuggestionTo fix this regression, the non-sharded write operation in `WriteData` should be updated to explicitly specify the target table, making it independent of the state of `c.db`. This will make its behavior consistent with the sharded write path.
// in pumps/sql.go, inside WriteData()
if !c.SQLConf.TableSharding {
	recs := make([]*analytics.AnalyticsRecord, 0, dataLen)
	for _, r := range data {
		recs = append(recs, r.(*analytics.AnalyticsRecord))
	}
	tx := c.db.WithContext(ctx).Table(analytics.SQLTable).Create(recs)
	if tx.Error != nil {
		c.log.Error(tx.Error)
	}
}
\n\n \n\n

Powered by Visor from Probelabs

Last updated: 2026-03-10T06:51:42.579Z | Triggered by: pr_opened | Commit: e39bc8d

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

@probelabs probelabs bot changed the title fix: GORM v2 table sharding bug fix: TT-16778 GORM v2 table sharding bug Mar 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🚨 Jira Linter Failed

Commit: e39bc8d
Failed at: 2026-03-10 07:21:13 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: branch name 'fix/gorm-v2-sharding-bug' must contain 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.

@probelabs
Copy link
Copy Markdown
Contributor Author

probelabs bot commented Mar 10, 2026

Superseded by #949

@probelabs probelabs bot closed this Mar 10, 2026
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.

1 participant