fix(temporal): fix workflow id too long error #304
Conversation
|
Warning Rate limit exceeded@paul-nicolas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Connector as Connector Model
participant Storage as Database
participant Migration as Migration Process
Connector->>Storage: Add Reference Column
Migration->>Storage: Populate Reference for Existing Connectors
Storage-->>Migration: Confirm Reference Population
Connector->>Connector: Update JSON Marshaling
Connector->>Connector: Modify Task ID Generation
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
3da547a to
46bcbd6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
internal/storage/connectors.go (2)
Line range hint
193-204: Add Reference field to returned Connector model.The ConnectorsGet method doesn't include the Reference field in the returned Connector model, which could cause issues if code depends on this field.
Apply this diff:
return &models.Connector{ ID: connector.ID, + Reference: connector.Reference, Name: connector.Name, CreatedAt: connector.CreatedAt.Time, Provider: connector.Provider, Config: connector.DecryptedConfig, ScheduledForDeletion: connector.ScheduledForDeletion, }, nil
Line range hint
251-267: Add Reference field to returned Connectors in list.Similar to ConnectorsGet, the ConnectorsList method doesn't include the Reference field in the returned Connector models.
Apply this diff:
for _, c := range cursor.Data { connectors = append(connectors, models.Connector{ ID: c.ID, + Reference: c.Reference, Name: c.Name, CreatedAt: c.CreatedAt.Time, Provider: c.Provider, Config: c.DecryptedConfig, ScheduledForDeletion: c.ScheduledForDeletion, }) }
🧹 Nitpick comments (1)
internal/storage/migrations/11-add-reference-for-connector.go (1)
18-25: Consider adding an index on the reference column.Since the reference column is used in the TaskIDReference function, adding an index would improve query performance when looking up connectors by reference.
Add this to the migration:
_, err := db.ExecContext(ctx, ` ALTER TABLE connectors ADD COLUMN IF NOT EXISTS reference uuid; + CREATE INDEX IF NOT EXISTS idx_connectors_reference ON connectors(reference); `)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
openapi.yamlis excluded by!**/*.yamlopenapi/v3/v3-schemas.yamlis excluded by!**/*.yaml
📒 Files selected for processing (7)
internal/models/connectors.go(1 hunks)internal/models/task_id.go(1 hunks)internal/storage/connectors.go(3 hunks)internal/storage/migrations/11-add-reference-for-connector.go(1 hunks)internal/storage/migrations/migrations.go(1 hunks)internal/storage/migrations/migrations_test.go(2 hunks)test/e2e/api_bank_accounts_test.go(1 hunks)
🔇 Additional comments (8)
internal/models/task_id.go (1)
18-18: LGTM! Verify backward compatibility of the new task ID format.The change reduces the task ID length by using UUID reference instead of the full connector ID string, which should fix the "workflow id too long" error while maintaining uniqueness.
Let's verify the impact on existing task IDs:
✅ Verification successful
✅ Task ID format change is backward compatible
The new format maintains uniqueness while being shorter:
- All callers provide full ConnectorID object
- Function signature remains unchanged
- New format separates provider from reference for better clarity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing task ID usage patterns rg -A 2 'TaskIDReference\(' .Length of output: 1737
internal/storage/migrations/migrations.go (1)
159-169: LGTM! Migration is properly registered with transaction support.The migration is well-structured with proper transaction handling and logging.
internal/storage/connectors.go (2)
38-38: LGTM! Reference field properly added to connector struct.The Reference field is correctly added with appropriate type and constraints.
104-104: LGTM! Reference properly populated during installation.The Reference field is correctly populated from the connector ID during installation.
test/e2e/api_bank_accounts_test.go (1)
139-140: LGTM! Test correctly validates the new reference-based task ID format.The test modification properly verifies that task IDs now include the connector's reference string, which aligns with the workflow ID length fix.
internal/models/connectors.go (1)
31-31: LGTM! Reference field addition is well-implemented.The Reference field is correctly added to the JSON representation and properly populated from the connector ID's reference.
Let's verify the impact on API consumers:
Also applies to: 39-39
✅ Verification successful
Reference field is read-only and safe to add
The Reference field is correctly implemented as a read-only, derived field that:
- Is populated during JSON serialization from the existing ID field
- Is not required during deserialization
- Has no impact on API consumers as it's computed from existing data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct JSON unmarshaling of Connector type # to ensure consumers can handle the new field rg -A 5 'json.Unmarshal.*Connector'Length of output: 37
Script:
#!/bin/bash # Search for Connector type usage in JSON context echo "=== Searching for JSON handling of Connector type ===" rg -g '!*.md' -i "json.*connector" echo -e "\n=== Searching for Connector type in test files ===" rg -g '*_test.go' "Connector" echo -e "\n=== Looking for custom JSON methods on Connector ===" ast-grep --pattern 'func ($_ *Connector) MarshalJSON() ([]byte, error)' ast-grep --pattern 'func ($_ *Connector) UnmarshalJSON([]byte) error'Length of output: 75004
Script:
#!/bin/bash # Look for Connector type definition and its JSON methods echo "=== Searching for Connector type definition ===" rg -A 10 "type Connector struct" echo -e "\n=== Searching for Connector JSON methods ===" rg "func.*Connector.*JSON"Length of output: 1169
Script:
#!/bin/bash # Get the implementation of Connector's JSON methods echo "=== Connector JSON method implementations ===" rg -A 15 "func \(c .*Connector\).*JSON" internal/models/connectors.goLength of output: 1437
internal/storage/migrations/migrations_test.go (2)
355-381: LGTM! Comprehensive migration test for connector references.The test thoroughly validates that all connector types have correct reference values in the database after migration. Good error handling and assertions.
80-80: LGTM! Migration test properly registered.The reference connector migration test is correctly registered in the test suite.
46bcbd6 to
c876bc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/storage/migrations/migrations_test.go (1)
354-381: Consider enhancing test coverage with negative cases.While the test effectively verifies the successful migration of connector references, it could be improved by:
- Testing for non-existent connectors
- Validating the format of the reference UUIDs
- Adding assertions for the total count of migrated records
Consider adding these test cases:
func testReferenceConnectorMigrations() testmigrations.Hook { return testmigrations.Hook{ After: func(ctx context.Context, t *testing.T, db bun.IDB) { + // Verify total count matches expected + count, err := db.NewSelect().TableExpr("connectors").Count(ctx) + require.NoError(t, err) + require.Equal(t, 10, count) + for _, connectorID := range []string{ // ... existing IDs ... } { id := models.MustConnectorIDFromString(connectorID) + + // Validate UUID format + require.Regexp(t, "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$", id.Reference.String()) exists, err := db.NewSelect().TableExpr("connectors"). Where("id = ?", connectorID). Where("reference = ?", id.Reference). Exists(ctx) require.NoError(t, err) require.True(t, exists) } + + // Test non-existent connector + exists, err := db.NewSelect().TableExpr("connectors"). + Where("id = ?", "invalid-connector-id"). + Exists(ctx) + require.NoError(t, err) + require.False(t, exists) }, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
openapi.yamlis excluded by!**/*.yamlopenapi/v3/v3-schemas.yamlis excluded by!**/*.yaml
📒 Files selected for processing (7)
internal/models/connectors.go(1 hunks)internal/models/task_id.go(1 hunks)internal/storage/connectors.go(3 hunks)internal/storage/migrations/11-add-reference-for-connector.go(1 hunks)internal/storage/migrations/migrations.go(1 hunks)internal/storage/migrations/migrations_test.go(2 hunks)test/e2e/api_bank_accounts_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/storage/connectors.go
- internal/storage/migrations/migrations.go
- internal/models/connectors.go
- internal/models/task_id.go
- test/e2e/api_bank_accounts_test.go
- internal/storage/migrations/11-add-reference-for-connector.go
🔇 Additional comments (2)
internal/storage/migrations/migrations_test.go (2)
80-80: LGTM! Test integration looks good.The new migration test is correctly integrated into the test suite with an appropriate sequence number.
370-370: Consider handling potential panic.The
MustConnectorIDFromStringfunction will panic if the connector ID is invalid. While this might be acceptable in tests, it's good practice to handle potential errors gracefully.Let's verify if there's error handling in the
MustConnectorIDFromStringfunction:✅ Verification successful
Using MustConnectorIDFromString is appropriate in test code
The use of
MustConnectorIDFromStringis acceptable in this context because:
- This is test code where panics are acceptable and help identify issues quickly
- The connector ID is a hardcoded, known-valid test value
- This follows the common Go testing pattern where "Must" variants are used in tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of ConnectorIDFromString to check error handling ast-grep --pattern 'func $_(s string) ($_, error) { $$$ }'Length 10000 of output: 3640
Script:
#!/bin/bash # Search for both ConnectorIDFromString and MustConnectorIDFromString implementations ast-grep --pattern 'func ConnectorIDFromString(s string) ($_, error) { $$$ }' ast-grep --pattern 'func MustConnectorIDFromString(s string) $_ { $$$ }'Length of output: 158
Script:
#!/bin/bash # Search for both function names with context rg -A 5 "ConnectorIDFromString" || true rg -A 5 "MustConnectorIDFromString" || trueLength of output: 22325
| @@ -28,13 +28,15 @@ func (c *Connector) IdempotencyKey() string { | |||
| func (c Connector) MarshalJSON() ([]byte, error) { | |||
| return json.Marshal(&struct { | |||
| ID string `json:"id"` | |||
| Reference string `json:"reference"` | |||
There was a problem hiding this comment.
Shouldn't you add it to the unmarshal json func if you're adding it here?
There was a problem hiding this comment.
Since it's not present in the struct model, I think adding it to the unmarshal would be useless since we will not do anything with it !
There was a problem hiding this comment.
But it should be symmetrical either way. If later we start using it it will be easier to create bugs because we forget there are some unaccounted for fields.
c876bc8 to
91ff6cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/storage/migrations/migrations_test.go (1)
354-381: Consider enhancing test coverage and error handling.While the basic implementation is solid, consider these improvements:
- Add negative test cases to verify that non-existent connectors are handled correctly
- Add test cases for malformed connector IDs
- Consider testing the actual reference values instead of just existence
Here's a suggested enhancement:
func testReferenceConnectorMigrations() testmigrations.Hook { return testmigrations.Hook{ After: func(ctx context.Context, t *testing.T, db bun.IDB) { + // Test valid connectors for _, connectorID := range []string{ "eyJQcm92aWRlciI6Im1vbmV5Y29ycCIsIlJlZmVyZW5jZSI6IjdkNGU1MjM3LTNjMDktNDUwZS04ODY5LTI2YzA2MGFmMjM3NyJ9", // ... other IDs ... } { id := models.MustConnectorIDFromString(connectorID) exists, err := db.NewSelect().TableExpr("connectors"). Where("id = ?", connectorID). Where("reference = ?", id.Reference). Exists(ctx) require.NoError(t, err) require.True(t, exists) + + // Verify the actual reference value + var reference string + err = db.NewSelect().TableExpr("connectors"). + Column("reference"). + Where("id = ?", connectorID). + Scan(ctx, &reference) + require.NoError(t, err) + require.Equal(t, id.Reference, reference) } + + // Test non-existent connector + exists, err := db.NewSelect().TableExpr("connectors"). + Where("id = ?", "non-existent-id"). + Exists(ctx) + require.NoError(t, err) + require.False(t, exists) + + // Test malformed connector ID + exists, err = db.NewSelect().TableExpr("connectors"). + Where("id = ?", "malformed-id"). + Exists(ctx) + require.NoError(t, err) + require.False(t, exists) }, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
openapi.yamlis excluded by!**/*.yamlopenapi/v3/v3-schemas.yamlis excluded by!**/*.yaml
📒 Files selected for processing (7)
internal/models/connectors.go(1 hunks)internal/models/task_id.go(1 hunks)internal/storage/connectors.go(3 hunks)internal/storage/migrations/11-add-reference-for-connector.go(1 hunks)internal/storage/migrations/migrations.go(1 hunks)internal/storage/migrations/migrations_test.go(2 hunks)test/e2e/api_bank_accounts_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/models/connectors.go
- internal/storage/migrations/migrations.go
- internal/storage/migrations/11-add-reference-for-connector.go
- internal/models/task_id.go
- internal/storage/connectors.go
- test/e2e/api_bank_accounts_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (1)
internal/storage/migrations/migrations_test.go (1)
80-80: LGTM! Test integration looks good.The new migration test is correctly integrated into the test suite with an appropriate sequence number.
No description provided.