Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds CreateStream handler with input validation for stream ID, stream type, and Ethereum data provider; introduces dbCreateStream to insert a stream row with a created_at Unix timestamp; and adds unit tests covering success, validation failures, duplicate-key mapping, and DB errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as CreateStreamHandler
participant Validator as Validators
participant DB as Database
Client->>Handler: Send CreateStream request
Handler->>Validator: validateDataProvider/validateStreamID/validateStreamType
alt validation fails
Validator-->>Handler: validation error
Handler-->>Client: return InvalidParams
else validation passes
Handler->>DB: dbCreateStream(insert stream row with created_at)
alt DB returns duplicate (23505 or duplicate-string)
DB-->>Handler: unique-violation
Handler-->>Client: return InvalidParams ("stream already exists")
else DB returns other error
DB-->>Handler: error
Handler-->>Client: return Internal error
else DB success
DB-->>Handler: success
Handler-->>Client: return CreateStreamResponse (empty)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extensions/tn_local/tn_local_test.go (1)
148-289: Add a nil-request test case forCreateStream.Given the handler currently dereferences
reqdirectly, a dedicated test forext.CreateStream(ctx, nil)would prevent panic regressions once the guard is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_local/tn_local_test.go` around lines 148 - 289, Add a new unit test named TestCreateStream_NilRequest that calls ext.CreateStream(context.Background(), nil) (using ext := newTestExtension(&utils.MockDB{})) and asserts the call does not panic, returns a non-nil rpcErr, and that rpcErr.Message contains a short, clear indication the request was nil (e.g., contains "request" or "nil"); this will catch the current dereference-of-req bug in CreateStream and validate the future nil-guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/tn_local/handlers.go`:
- Around line 42-44: The CreateStream handler currently dereferences req without
checking for nil, which can panic; update Extension.CreateStream to first check
if req == nil and return a jsonrpc.NewError(jsonrpc.ErrorInvalidParams, "missing
request", nil) (or a suitable error) before calling
validateDataProvider(req.DataProvider), ensuring all subsequent uses of req
(e.g., req.DataProvider) occur after the nil guard.
- Around line 53-56: The duplicate-key detection around the ext.dbCreateStream
error handling is too brittle because it only matches exact lowercase
substrings; change the detection in the error handling for ext.dbCreateStream to
robustly identify unique-constraint/duplicate-key errors (so you still return
jsonrpc.ErrorInvalidParams for duplicates) by either inspecting
database-specific error types/codes (e.g., detect SQLState "23505" for Postgres
via errors.As or the MySQL/SQLite error codes) and/or performing a
case-insensitive/regex match (e.g., lowercasing the error string or using a
compiled regexp for "duplicate|unique|already exists"), update the logic that
currently uses strings.Contains(err.Error(), "duplicate key") ||
strings.Contains(err.Error(), "unique constraint") so it covers different
drivers/formats and still returns the same jsonrpc.NewError(... "stream already
exists: %s/%s") on duplicate detection.
---
Nitpick comments:
In `@extensions/tn_local/tn_local_test.go`:
- Around line 148-289: Add a new unit test named TestCreateStream_NilRequest
that calls ext.CreateStream(context.Background(), nil) (using ext :=
newTestExtension(&utils.MockDB{})) and asserts the call does not panic, returns
a non-nil rpcErr, and that rpcErr.Message contains a short, clear indication the
request was nil (e.g., contains "request" or "nil"); this will catch the current
dereference-of-req bug in CreateStream and validate the future nil-guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d40b92a6-329d-4077-9487-859ce64747a3
📒 Files selected for processing (3)
extensions/tn_local/db_ops.goextensions/tn_local/handlers.goextensions/tn_local/tn_local_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
extensions/tn_local/tn_local_test.go (1)
265-280: Add a test for the*pgconn.PgErrorduplicate path.Current duplicate coverage validates only the fallback string matcher. Add one test using a typed PostgreSQL error (
Code: "23505") so the primary branch inisDuplicateKeyErroris exercised too.🧪 Suggested test addition
import ( "context" "fmt" "io" "strings" "sync" "testing" + "github.com/jackc/pgx/v5/pgconn" "github.com/stretchr/testify/require" "github.com/trufnetwork/kwil-db/core/log" jsonrpc "github.com/trufnetwork/kwil-db/core/rpc/json" kwilsql "github.com/trufnetwork/kwil-db/node/types/sql" "github.com/trufnetwork/node/tests/utils" ) @@ func TestCreateStream_DuplicateStream(t *testing.T) { @@ } + +func TestCreateStream_DuplicateStreamPgError(t *testing.T) { + mockDB := &utils.MockDB{ + ExecuteFn: func(ctx context.Context, stmt string, args ...any) (*kwilsql.ResultSet, error) { + return nil, &pgconn.PgError{Code: pgUniqueViolation, Message: "duplicate key value violates unique constraint"} + }, + } + ext := newTestExtension(mockDB) + + _, rpcErr := ext.CreateStream(context.Background(), &CreateStreamRequest{ + DataProvider: "0xEC36224A679218Ae28FCeCe8d3c68595B87Dd832", + StreamID: "st00000000000000000000000000test", + StreamType: "primitive", + }) + require.NotNil(t, rpcErr) + require.Equal(t, jsonrpc.ErrorCode(jsonrpc.ErrorInvalidParams), rpcErr.Code) + require.Contains(t, rpcErr.Message, "stream already exists") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_local/tn_local_test.go` around lines 265 - 280, TestCreateStream_DuplicateStream only exercises the generic string error path; add a new test (or extend this one) that makes mockDB.ExecuteFn return a typed *pgconn.PgError with Code "23505" so the primary branch in isDuplicateKeyError is exercised; in the test create ext := newTestExtension(mockDB), call ext.CreateStream(...) with the same request used here, assert rpcErr is not nil and rpcErr.Message contains "stream already exists", and ensure the mock returns the pgconn.PgError from the ExecuteFn (referencing TestCreateStream_DuplicateStream, newTestExtension, CreateStream, isDuplicateKeyError, and MockDB.ExecuteFn to locate where to implement the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/tn_local/handlers.go`:
- Around line 62-75: Normalize req.DataProvider before validation and DB insert
to avoid case-variant duplicates: convert the incoming req.DataProvider to a
canonical form (e.g., strings.ToLower(req.DataProvider) or an EIP-55
checksum-normalized value) and then use that normalized value for
validateDataProvider calls, any error messages, and the ext.dbCreateStream call
(replace usages of req.DataProvider with the normalized variable); ensure the
same normalized value is used when constructing the duplicate-key error message
so uniqueness checks and logs align.
---
Nitpick comments:
In `@extensions/tn_local/tn_local_test.go`:
- Around line 265-280: TestCreateStream_DuplicateStream only exercises the
generic string error path; add a new test (or extend this one) that makes
mockDB.ExecuteFn return a typed *pgconn.PgError with Code "23505" so the primary
branch in isDuplicateKeyError is exercised; in the test create ext :=
newTestExtension(mockDB), call ext.CreateStream(...) with the same request used
here, assert rpcErr is not nil and rpcErr.Message contains "stream already
exists", and ensure the mock returns the pgconn.PgError from the ExecuteFn
(referencing TestCreateStream_DuplicateStream, newTestExtension, CreateStream,
isDuplicateKeyError, and MockDB.ExecuteFn to locate where to implement the
change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82f3ea6d-f24e-4422-ab18-e90523882c78
📒 Files selected for processing (2)
extensions/tn_local/handlers.goextensions/tn_local/tn_local_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
extensions/tn_local/handlers.go (1)
47-55: Defensively handlenilinisDuplicateKeyError.Current callers pass non-nil errors, but this helper will panic if reused with
nilbecause oferr.Error(). A small guard keeps it future-proof.Suggested patch
func isDuplicateKeyError(err error) bool { + if err == nil { + return false + } var pgErr *pgconn.PgError if errors.As(err, &pgErr) { return pgErr.Code == pgUniqueViolation } // Fallback for non-pgx drivers (e.g. mocks): case-insensitive string match. msg := strings.ToLower(err.Error()) return strings.Contains(msg, "duplicate key") || strings.Contains(msg, "unique constraint") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_local/handlers.go` around lines 47 - 55, The helper isDuplicateKeyError can panic on a nil err; add a nil guard at the top (e.g., if err == nil return false) so it safely returns false for nil inputs, then retain the existing errors.As check against pgErr and the fallback string checks (pgUniqueViolation, pgErr) unchanged.extensions/tn_local/tn_local_test.go (2)
287-287: UsepgUniqueViolationconstant in test instead of a literal code.This keeps test and handler logic aligned if the code identifier changes.
Suggested patch
- return nil, &pgconn.PgError{Code: "23505", Message: "unique_violation"} + return nil, &pgconn.PgError{Code: pgUniqueViolation, Message: "unique_violation"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_local/tn_local_test.go` at line 287, Replace the hard-coded Postgres error code string in the test's mocked error (currently created as &pgconn.PgError{Code: "23505", ...}) with the shared constant pgUniqueViolation so the test uses the same identifier as the handler; update the return in the test stub that currently returns nil, &pgconn.PgError{...} to use pgUniqueViolation instead of the literal "23505".
149-156: Assert JSON-RPC error codes in validation tests, not only messages.These tests currently lock message text, but not the contract that validation failures map to
ErrorInvalidParams. Add code assertions to catch classification regressions.Suggested patch pattern
func TestCreateStream_NilRequest(t *testing.T) { ext := newTestExtension(&utils.MockDB{}) resp, rpcErr := ext.CreateStream(context.Background(), nil) require.Nil(t, resp) require.NotNil(t, rpcErr) + require.Equal(t, jsonrpc.ErrorCode(jsonrpc.ErrorInvalidParams), rpcErr.Code) require.Contains(t, rpcErr.Message, "missing request") }require.NotNil(t, rpcErr) + require.Equal(t, jsonrpc.ErrorCode(jsonrpc.ErrorInvalidParams), rpcErr.Code) require.Contains(t, rpcErr.Message, tt.wantMsg)Also applies to: 203-239, 241-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_local/tn_local_test.go` around lines 149 - 156, The tests (e.g., TestCreateStream_NilRequest) only assert the error message text but not the JSON-RPC error classification; update the validation tests to also assert the RPC error code equals the validation constant (e.g., ErrorInvalidParams / proto.ErrorInvalidParams) whenever a validation failure is expected. Locate test functions like TestCreateStream_NilRequest and the other validation test blocks around lines 203-239 and 241-265 and add an assertion that rpcErr.Code == proto.ErrorInvalidParams (or the project’s ErrorInvalidParams constant) in addition to the existing require.Contains on rpcErr.Message so regressions in error classification are caught. Ensure you import or reference the same error-code symbol used by the RPC layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extensions/tn_local/handlers.go`:
- Around line 47-55: The helper isDuplicateKeyError can panic on a nil err; add
a nil guard at the top (e.g., if err == nil return false) so it safely returns
false for nil inputs, then retain the existing errors.As check against pgErr and
the fallback string checks (pgUniqueViolation, pgErr) unchanged.
In `@extensions/tn_local/tn_local_test.go`:
- Line 287: Replace the hard-coded Postgres error code string in the test's
mocked error (currently created as &pgconn.PgError{Code: "23505", ...}) with the
shared constant pgUniqueViolation so the test uses the same identifier as the
handler; update the return in the test stub that currently returns nil,
&pgconn.PgError{...} to use pgUniqueViolation instead of the literal "23505".
- Around line 149-156: The tests (e.g., TestCreateStream_NilRequest) only assert
the error message text but not the JSON-RPC error classification; update the
validation tests to also assert the RPC error code equals the validation
constant (e.g., ErrorInvalidParams / proto.ErrorInvalidParams) whenever a
validation failure is expected. Locate test functions like
TestCreateStream_NilRequest and the other validation test blocks around lines
203-239 and 241-265 and add an assertion that rpcErr.Code ==
proto.ErrorInvalidParams (or the project’s ErrorInvalidParams constant) in
addition to the existing require.Contains on rpcErr.Message so regressions in
error classification are caught. Ensure you import or reference the same
error-code symbol used by the RPC layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f601211-ce1d-437d-9849-79889df17817
📒 Files selected for processing (2)
extensions/tn_local/handlers.goextensions/tn_local/tn_local_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
extensions/tn_local/tn_local_test.go (2)
159-184: Assertcreated_atargument in success path to fully cover DB contract.
dbCreateStreamwritescreated_at; this test currently validates only the first three args. Add arg-count and timestamp checks to prevent silent regressions in insert parameter order/shape.Proposed test hardening diff
import ( "context" "fmt" "io" "strings" "sync" "testing" + "time" @@ func TestCreateStream_Success(t *testing.T) { + before := time.Now().Unix() var capturedStmt string var capturedArgs []any mockDB := &utils.MockDB{ ExecuteFn: func(ctx context.Context, stmt string, args ...any) (*kwilsql.ResultSet, error) { capturedStmt = stmt @@ }) + after := time.Now().Unix() @@ require.Contains(t, capturedStmt, "INSERT INTO "+SchemaName+".streams") // data_provider should be lowercased (matching consensus behavior) + require.Len(t, capturedArgs, 4) require.Equal(t, "0xec36224a679218ae28fcece8d3c68595b87dd832", capturedArgs[0]) require.Equal(t, "st00000000000000000000000000test", capturedArgs[1]) require.Equal(t, "primitive", capturedArgs[2]) + createdAt, ok := capturedArgs[3].(int64) + require.True(t, ok, "created_at should be unix seconds (int64)") + require.GreaterOrEqual(t, createdAt, before) + require.LessOrEqual(t, createdAt, after) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_local/tn_local_test.go` around lines 159 - 184, The test TestCreateStream_Success currently only asserts the first three INSERT args and should be hardened to validate the created_at parameter so it matches the dbCreateStream contract: update the test to assert len(capturedArgs) is at least 4 (or exactly 4) and that capturedArgs[3] is a non-zero timestamp (or a time.Time within an acceptable range), keeping the existing checks for the lowercased data provider, stream id and stream type; ensure the INSERT target still contains "INSERT INTO "+SchemaName+".streams" and reference capturedArgs[3] as the created_at value to prevent silent regressions in parameter ordering/shape.
271-286: Duplicate-stream tests should also assert JSON-RPC error code.Both duplicate tests currently assert only the message. Add explicit code checks so handler-level error mapping changes are caught immediately.
Proposed assertion additions
require.NotNil(t, rpcErr) + require.Equal(t, jsonrpc.ErrorCode(jsonrpc.ErrorInvalidParams), rpcErr.Code) require.Contains(t, rpcErr.Message, "stream already exists") @@ require.NotNil(t, rpcErr) + require.Equal(t, jsonrpc.ErrorCode(jsonrpc.ErrorInvalidParams), rpcErr.Code) require.Contains(t, rpcErr.Message, "stream already exists")Also applies to: 288-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_local/tn_local_test.go` around lines 271 - 286, The test TestCreateStream_DuplicateStream currently only checks the error message; update it to also assert the JSON-RPC error code returned by CreateStream: after calling ext.CreateStream and verifying rpcErr is not nil, add an assertion that rpcErr.Code equals the handler's duplicate-stream JSON-RPC error constant (the same constant the CreateStream handler maps duplicate-key DB errors to, e.g. ErrStreamAlreadyExistsCode); make the same change in the sibling duplicate-stream test at the 288-303 region so both tests verify message and code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extensions/tn_local/tn_local_test.go`:
- Around line 159-184: The test TestCreateStream_Success currently only asserts
the first three INSERT args and should be hardened to validate the created_at
parameter so it matches the dbCreateStream contract: update the test to assert
len(capturedArgs) is at least 4 (or exactly 4) and that capturedArgs[3] is a
non-zero timestamp (or a time.Time within an acceptable range), keeping the
existing checks for the lowercased data provider, stream id and stream type;
ensure the INSERT target still contains "INSERT INTO "+SchemaName+".streams" and
reference capturedArgs[3] as the created_at value to prevent silent regressions
in parameter ordering/shape.
- Around line 271-286: The test TestCreateStream_DuplicateStream currently only
checks the error message; update it to also assert the JSON-RPC error code
returned by CreateStream: after calling ext.CreateStream and verifying rpcErr is
not nil, add an assertion that rpcErr.Code equals the handler's duplicate-stream
JSON-RPC error constant (the same constant the CreateStream handler maps
duplicate-key DB errors to, e.g. ErrStreamAlreadyExistsCode); make the same
change in the sibling duplicate-stream test at the 288-303 region so both tests
verify message and code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd1b9dc9-9c11-4d50-b4d5-5aec159d2cc3
📒 Files selected for processing (2)
extensions/tn_local/handlers.goextensions/tn_local/tn_local_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/tn_local/handlers.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves: https://github.com/truflation/website/issues/3477
Summary by CodeRabbit
New Features
Bug Fixes
Tests