Fix/tests blockwise requires message#627
Conversation
WalkthroughAdds remote address–aware blockwise processing by introducing an exchangeKey derived from token or remoteAddr, refactoring cache lookups and method signatures (Do/Handle) accordingly. Updates TCP/UDP clients and tests to pass remoteAddr. Adjusts example ports from 5688 to 5685, adds .gitignore entry for /.idea/, and introduces libcoap UDP integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (Client)
participant BW as BlockWise.Do
participant Key as getExchangeKey
participant Cache as SendingMessagesCache
participant Net as Transport.Do
App->>BW: Do(req, szx, maxMsg, remoteAddr, handler)
BW->>Key: getExchangeKey(req.Token, remoteAddr)
Key-->>BW: exchangeKey
BW->>Cache: registerSendingMessage(exchangeKey, req, expire)
alt payload fits
BW->>Net: handler(req)
Net-->>BW: resp/err
BW-->>App: resp/err
else blockwise (payload too large)
BW->>BW: buildFirstBlockRequest()
BW->>Net: handler(firstBlockReq)
Net-->>BW: resp/err
BW-->>App: resp/err
end
sequenceDiagram
autonumber
participant Net as Network
participant BW as BlockWise.Handle
participant Key as getExchangeKey
participant Cache as Exchanges (send/recv)
participant Next as Next Handler
Net->>BW: Handle(w, msg, szx, maxMsg, remoteAddr, next)
BW->>Key: getExchangeKey(msg.Token, remoteAddr)
Key-->>BW: exchangeKey
alt receiving full/non-blockwise
BW->>Next: next(w, msg)
Next-->>BW: done
else continue sending blocks
BW->>Cache: lookup(exchangeKey)
Cache-->>BW: state
BW->>BW: continueSendingMessage(...)
BW->>Next: next(w, assembled/nextBlockMsg)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
examples/simple/client/main.go (1)
13-13: Port change is consistent with the server.The dial target port has been updated from 5688 to 5685, matching the server's listening port. However, consider using the standard CoAP port 5683 for unsecured UDP, or document why 5685 was chosen (e.g., test isolation, avoiding conflicts).
examples/simple/server/main.go (1)
46-46: Port change is consistent with the client.The listening port has been updated from 5688 to 5685, matching the client's dial target. However, consider using the standard CoAP port 5683 for unsecured UDP, or document why 5685 was chosen (e.g., test isolation, avoiding conflicts).
udp/server_libcoap_test.go (7)
111-116: Drop else after return (revive indent-error-flow).- if tt.wantErr { - require.Error(t, err) - return - } else { - require.NoError(t, err) - } + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err)Also applies to: 216-221, 315-321
185-191: Avoid err shadowing inside handler (govet).- received, err := r.ReadBody() - assert.NoError(t, err) + received, errR := r.ReadBody() + assert.NoError(t, errR)
283-289: Avoid err shadowing inside handler (govet).- received, err := r.ReadBody() + received, errR := r.ReadBody() errS := w.SetResponse(codes.Content, message.TextPlain, bytes.NewReader([]byte(bigPayload10KiB))) - assert.NoError(t, err) + assert.NoError(t, errR)
332-336: Prefer errors.New for static error (perfsprint).- return nil, fmt.Errorf("coap-client not found in PATH") + return nil, errors.New("coap-client not found in PATH")Add imports:
- Add:
errorsto import list. After applying, run gci/gofumpt. [Based on lint hints]
341-343: Avoid unused rand and use strconv.FormatUint (perfsprint).- rand.Uint64() - args = append(args, "-T", fmt.Sprintf("%d", rand.Uint64())) + tkn := rand.Uint64() + args = append(args, "-T", strconv.FormatUint(tkn, 10))Add imports:
- Add:
strconvto import list. Run gci/gofumpt.
77-84: GET handler returns AppOctets but tests expect TextPlain.Either align content format or remove unused wantContentFormat field.
96-103: Remove unused local UDP client cc to reduce flakiness.It’s not used by the test; keeping it adds noise.
Also applies to: 204-211, 303-310
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)examples/simple/client/main.go(1 hunks)examples/simple/server/main.go(1 hunks)net/blockwise/blockwise.go(19 hunks)net/blockwise/blockwise_test.go(3 hunks)tcp/client/conn.go(2 hunks)udp/client/conn.go(2 hunks)udp/server_libcoap_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
examples/simple/client/main.go (1)
udp/client.go (1)
Dial(25-40)
udp/client/conn.go (2)
tcp/client/conn.go (1)
Conn(44-57)mux/client.go (1)
Conn(17-51)
examples/simple/server/main.go (1)
server.go (1)
ListenAndServe(23-52)
tcp/client/conn.go (1)
udp/client/conn.go (1)
Session(51-68)
net/blockwise/blockwise.go (3)
message/pool/message.go (1)
Message(26-41)message/getToken.go (1)
Token(9-9)pkg/cache/cache.go (2)
NewElement(32-39)Element(14-18)
udp/server_libcoap_test.go (6)
message/type.go (2)
Type(11-11)Confirmable(17-17)message/option.go (3)
MediaType(162-162)TextPlain(166-166)AppOctets(172-172)message/codes/codes.go (5)
Content(29-29)GET(16-16)Changed(28-28)PUT(18-18)POST(17-17)net/connUDP.go (1)
NewListenUDP(233-243)mux/router.go (1)
NewRouter(67-83)options/commonOptions.go (1)
WithMux(120-124)
net/blockwise/blockwise_test.go (1)
net/responsewriter/responseWriter.go (2)
New(23-38)ResponseWriter(17-21)
🪛 GitHub Actions: Golangci-lint
udp/server_libcoap_test.go
[error] 361-361: golangci-lint: non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint).
🪛 GitHub Actions: Test for forked repository
udp/client/conn.go
[error] 1-1: CI run failed: Process completed with exit code 1.
udp/server_libcoap_test.go
[error] 1-1: TestConnGetBlockwise failed: coap-client not found in PATH
[error] 1-1: TestConnPutBlockwise failed: coap-client not found in PATH
[error] 1-1: TestConnPostBlockwise failed: coap-client not found in PATH
[error] 1-1: CI run failed: Process completed with exit code 1.
🪛 GitHub Check: lint
udp/server_libcoap_test.go
[failure] 318-318:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
[failure] 219-219:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
[failure] 114-114:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
[failure] 342-342:
integer-format: fmt.Sprintf can be replaced with faster strconv.FormatUint (perfsprint)
[failure] 334-334:
error-format: fmt.Errorf can be replaced with errors.New (perfsprint)
[failure] 283-283:
shadow: declaration of "err" shadows declaration at line 264 (govet)
[failure] 185-185:
shadow: declaration of "err" shadows declaration at line 165 (govet)
[failure] 20-20:
File is not properly formatted (gofumpt)
[failure] 5-5:
File is not properly formatted (gci)
[failure] 361-361:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Fuzzing
🔇 Additional comments (9)
.gitignore (1)
14-14: Ignore IDE metadataAdding
/.idea/keeps JetBrains project files out of version control—looks good.net/blockwise/blockwise_test.go (1)
80-86: Signature updates look correct.Passing nil for remoteAddr in tests that always use non-empty tokens is fine.
Also applies to: 83-86, 300-303, 491-500, 499-506, 554-572, 571-579
tcp/client/conn.go (1)
210-214: Correctly propagates remote address into blockwise.Also applies to: 355-356
udp/client/conn.go (1)
443-449: RemoteAddr pass-through is correct and consistent.Also applies to: 666-673
net/blockwise/blockwise.go (5)
290-328: Do: validate + register + first-block flow looks solid.Input validation, cache registration, and first-block handling are correct. Defer delete ensures cleanup.
407-439: Handle: exchangeKey routing and branching is correct.Derives exchangeKey, picks receive vs. send continuation path, and cleans cache when done.
592-626: startSendingMessage: safe fallback and caching.Good handling of small payload bypass, observe special-case, and token fallback when exchangeKey==0.
910-925: getExchangeKey: simple and effective.Token preference with FNV64a(remoteAddr) fallback matches RFC guidance for empty tokens.
820-872: processReceivedMessage: exchangeKey plumbing + observe rekeying.Correctly switches to token-based key on observe re-tokening; cleans caches on completion.
| // verify that the binary coap-client is installed | ||
| _, err := exec.LookPath("coap-client") | ||
| if err != nil { | ||
| t.Error("coap-client not found in PATH") | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Skip tests when coap-client is missing (fixes CI).
Use t.Skip instead of t.Error so forks without libcoap don’t fail.
- _, err := exec.LookPath("coap-client")
- if err != nil {
- t.Error("coap-client not found in PATH")
- return
- }
+ if _, err := exec.LookPath("coap-client"); err != nil {
+ t.Skip("coap-client not found in PATH")
+ }Also applies to: 165-169, 264-269
🤖 Prompt for AI Agents
In udp/server_libcoap_test.go around lines 59-65 (and similarly at 165-169 and
264-269), the test currently fails the suite when the external binary
coap-client is missing by calling t.Error; change these to t.Skip (with a short
message like "coap-client not found in PATH; skipping test") so CI forks without
libcoap will skip the tests instead of failing. Replace the t.Error + return
with a single t.Skipf or t.Skip call and remove the explicit return since t.Skip
halts the test.
| l, err := coapNet.NewListenUDP("udp", "") | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| errC := l.Close() | ||
| require.NoError(t, errC) | ||
| }() |
There was a problem hiding this comment.
Bind server to loopback to produce a valid client URI.
Binding to "" may yield coap://[::]:port which is not a routable destination. Use 127.0.0.1:0.
- l, err := coapNet.NewListenUDP("udp", "")
+ l, err := coapNet.NewListenUDP("udp", "127.0.0.1:0")Also applies to: 171-176, 270-275
🤖 Prompt for AI Agents
In udp/server_libcoap_test.go around lines 66-71 (and also apply the same change
at 171-176 and 270-275), the test binds the server to "" which can produce an
unroutable coap://[::]:port client URI; change the bind address to "127.0.0.1:0"
when calling coapNet.NewListenUDP so the server listens on loopback and yields a
valid client URI (update the three NewListenUDP calls accordingly and adjust any
dependent variables if necessary).
| err = cmd.Run() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("coap-client error: %v, output: %s", err, out.String()) | ||
| } |
There was a problem hiding this comment.
Wrap underlying error with %w (errorlint).
- return nil, fmt.Errorf("coap-client error: %v, output: %s", err, out.String())
+ return nil, fmt.Errorf("coap-client error: %w; output: %s", err, out.String())🧰 Tools
🪛 GitHub Actions: Golangci-lint
[error] 361-361: golangci-lint: non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint).
🪛 GitHub Check: lint
[failure] 361-361:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
🤖 Prompt for AI Agents
In udp/server_libcoap_test.go around lines 359 to 362, the returned fmt.Errorf
call wraps the underlying cmd.Run() error using %v instead of using %w; change
the error construction to wrap the original error with %w so callers can
inspect/unwrap it (while still including the command output text for context) —
update the fmt.Errorf format to use %w and pass err as the wrapped error.
4a27d0a
into
plgd-dev:jkralik/fix/blockwise-requires-Message-ID
Extends with unit tests using libcoap coap-client: #626 See discussion there
Summary by CodeRabbit