Add configurable HTTP verb and update BlueFlow field names#23
Add configurable HTTP verb and update BlueFlow field names#23t-a-y-l-o-r wants to merge 7 commits into
Conversation
Use rune() cast in hl7_decode_test.go to fix build error with modern Go. Add go.mod/go.sum to enable module-aware builds.
Add -httpverb flag (default PUT, allows POST) to configure the HTTP method used when calling the BlueFlow upsert API. Closes #22.
ipv4_address → ip_address, identifier → name. Updates JSON tags and CSV headers to use canonical BlueFlow field names.
Change ListensOnPort (string) to ListensOnPorts ([]int) with JSON tag open_ports_tcp to match BlueFlow serializer. Update api.go package comment to be HTTP method-agnostic.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 6 seconds. ⌛ 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. 📝 WalkthroughWalkthroughThe changes implement configurable HTTP verb support for the BlueFlow upsert endpoint (defaulting to PUT with POST fallback), update the API payload schema to use canonical field names, and refactor the Asset model to represent listening ports as an integer slice rather than a single string, with coordinated updates across API, packet processing, and statistics modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Packets where the Ethernet layer fails to decode produce assets with empty MAC addresses. Uploading these is useless — BlueFlow cannot upsert without a MAC — and the new upsert endpoint rejects them with 400. Guard the upload path so we only send assets with a MAC.
Go nil slices serialize as null in JSON, which BlueFlow's ListField
rejects. Add NewAsset() constructor that initializes ListensOnPorts
to []int{} so it serializes as [] instead.
![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 (5)
api_test.go (1)
35-35: Assert the request verb and add a POST case.These tests now pass
"PUT", but nothing in the handler checksr.Method, so a regression back to a hardcoded verb would still pass. Please assert PUT here and add one POST variant to lock down the new feature.Suggested test tightening
mux.HandleFunc("/api", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPut { + t.Fatalf("expected %s, got %s", http.MethodPut, r.Method) + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) w.Write([]byte(`{"message": "Dummy message."}`)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_test.go` at line 35, The test currently constructs apiClient via NewAPIClient(..., "PUT", ...) but the test handler never asserts r.Method, so restore strictness by adding an assertion in the test HTTP handler that r.Method == "PUT" (use the request object passed to the handler; e.g., if the handler closure references r, call require.Equal(t, "PUT", r.Method) or t.Fatalf on mismatch). Then add a second subtest/case that constructs apiClient with "POST" (NewAPIClient(..., "POST", ...)) and a handler assertion that r.Method == "POST" to ensure both verbs are exercised and the client honors the configured verb.asset_test.go (1)
21-21: Please add a multi-port CSV case here.The serializer now joins
open_ports_tcpwith;, but this test still only covers a one-element slice. A regression that drops every port after the first would still pass.Also applies to: 45-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asset_test.go` at line 21, Add a test case in asset_test.go that verifies multi-port CSV serialization for the open_ports_tcp field: replace or extend the single-port case []int{8000} with a multi-value slice (e.g., []int{80, 443, 8080}) and assert that the serializer/joiner outputs "80;443;8080" (not just the first element); do the same update for the other occurrences noted (the similar cases around the other test blocks) so the test covers joining multiple ports with ';'.stats_test.go (1)
25-25: Add one multi-port fixture to cover the new counting path.All of these updated assets still use a single listening port, so
Stats.AddAsset()'s new slice iteration never gets exercised with[]int{...,...}. A regression that only counts the first entry would still pass this file.Also applies to: 83-83, 95-95, 133-133, 145-145, 183-183, 195-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stats_test.go` at line 25, The test fixtures all use single-port slices like []int{8000}, so the new iteration path in Stats.AddAsset() never gets exercised; update one (or more) of the fixtures in stats_test.go to use a multi-port slice (e.g. change one []int{8000} to []int{8000, 8001}) so the slice-iteration logic in Stats.AddAsset() is covered; apply the same multi-port change to at least one of the other occurrences mentioned (the other []int{...} fixtures) so the regression that only counts the first port would fail.api.go (1)
48-56: Document or enforce the HTTP method contract locally within NewAPIClient.The function currently accepts any string for
httpMethodwithout validation, relying on the CLI flag parser in main.go to enforce PUT/POST constraints before calling NewAPIClient. While this validation exists in the primary code path (main.go lines 128-130), the function itself creates an implicit contract that is not documented and not enforced. If future call sites bypass CLI parsing or new entry points are added, invalid values could slip through.Consider either:
- Add a comment documenting the expected contract (PUT or POST only)
- Implement local validation in NewAPIClient to be defensive
Also applies to: lines 91-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api.go` around lines 48 - 56, NewAPIClient currently accepts any string for the httpMethod parameter; add a local defensive check in NewAPIClient to enforce the allowed methods (only "PUT" or "POST") and normalize input (e.g., strings.ToUpper), and if the value is not one of those two, fall back to a safe default like "POST" and (optionally) log a warning; also add a short comment above NewAPIClient documenting the PUT/POST contract so future callers know the expectation.main.go (1)
123-130: Normalize-httpverbbefore validating it.Line 128 currently rejects harmless inputs like
postorput. Accepting case-insensitive input will make the flag less brittle without changing behavior.Suggested tweak
+ normalizedHTTPVerb := strings.ToUpper(strings.TrimSpace(*httpVerb)) - if *httpVerb != "PUT" && *httpVerb != "POST" { + if normalizedHTTPVerb != "PUT" && normalizedHTTPVerb != "POST" { fmt.Fprintf(os.Stderr, "Invalid -httpverb %q: must be PUT or POST\n", *httpVerb) os.Exit(1) } + *httpVerb = normalizedHTTPVerbAlso add
stringsto the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 123 - 130, Normalize the -httpverb value before validating by trimming whitespace and converting to upper case (operate on the pointer value returned by flag.String, i.e., httpVerb) and then check that the normalized value is "PUT" or "POST"; update the validation block that currently uses *httpVerb directly and add the "strings" import so you can call strings.TrimSpace and strings.ToUpper when validating and using the HTTP method elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asset.go`:
- Around line 12-17: NewAsset() alone isn't enough because callers can create
&Asset{} leaving ListensOnPorts nil; add enforcement at marshal time by
implementing Asset.MarshalJSON (or a prepare-for-upload helper called before
UploadAsset) to ensure ListensOnPorts is serialized as [] rather than null:
detect nil ListensOnPorts inside the custom MarshalJSON (or PrepareForUpload)
for type Asset and replace it with an empty slice before delegating to
json.Marshal so any direct &Asset{} or new(Asset) will still produce
"open_ports_tcp": [].
In `@go.mod`:
- Line 3: Update the module's Go version directive by changing the go directive
value from "go 1.26.1" to "go 1.26" in the go.mod (i.e., the go directive line)
to avoid pinning a patch-level version; if you truly require the 1.26.1 patch,
instead add a toolchain directive (e.g., toolchain go1.26.1) rather than locking
the go directive.
---
Nitpick comments:
In `@api_test.go`:
- Line 35: The test currently constructs apiClient via NewAPIClient(..., "PUT",
...) but the test handler never asserts r.Method, so restore strictness by
adding an assertion in the test HTTP handler that r.Method == "PUT" (use the
request object passed to the handler; e.g., if the handler closure references r,
call require.Equal(t, "PUT", r.Method) or t.Fatalf on mismatch). Then add a
second subtest/case that constructs apiClient with "POST" (NewAPIClient(...,
"POST", ...)) and a handler assertion that r.Method == "POST" to ensure both
verbs are exercised and the client honors the configured verb.
In `@api.go`:
- Around line 48-56: NewAPIClient currently accepts any string for the
httpMethod parameter; add a local defensive check in NewAPIClient to enforce the
allowed methods (only "PUT" or "POST") and normalize input (e.g.,
strings.ToUpper), and if the value is not one of those two, fall back to a safe
default like "POST" and (optionally) log a warning; also add a short comment
above NewAPIClient documenting the PUT/POST contract so future callers know the
expectation.
In `@asset_test.go`:
- Line 21: Add a test case in asset_test.go that verifies multi-port CSV
serialization for the open_ports_tcp field: replace or extend the single-port
case []int{8000} with a multi-value slice (e.g., []int{80, 443, 8080}) and
assert that the serializer/joiner outputs "80;443;8080" (not just the first
element); do the same update for the other occurrences noted (the similar cases
around the other test blocks) so the test covers joining multiple ports with
';'.
In `@main.go`:
- Around line 123-130: Normalize the -httpverb value before validating by
trimming whitespace and converting to upper case (operate on the pointer value
returned by flag.String, i.e., httpVerb) and then check that the normalized
value is "PUT" or "POST"; update the validation block that currently uses
*httpVerb directly and add the "strings" import so you can call
strings.TrimSpace and strings.ToUpper when validating and using the HTTP method
elsewhere.
In `@stats_test.go`:
- Line 25: The test fixtures all use single-port slices like []int{8000}, so the
new iteration path in Stats.AddAsset() never gets exercised; update one (or
more) of the fixtures in stats_test.go to use a multi-port slice (e.g. change
one []int{8000} to []int{8000, 8001}) so the slice-iteration logic in
Stats.AddAsset() is covered; apply the same multi-port change to at least one of
the other occurrences mentioned (the other []int{...} fixtures) so the
regression that only counts the first port would fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0115ac63-eac3-4f37-830c-44ad16f5af51
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
api.goapi_test.goasset.goasset_test.gogo.modhl7_decode_test.gomain.gopacket.gopacket_test.gostats.gostats_test.go
| // NewAsset creates an Asset with safe defaults (e.g., empty slice instead of nil | ||
| // for ListensOnPorts so it serializes as [] rather than null in JSON). | ||
| func NewAsset() *Asset { | ||
| return &Asset{ | ||
| ListensOnPorts: []int{}, | ||
| } |
There was a problem hiding this comment.
NewAsset() alone doesn't guarantee open_ports_tcp serializes as [].
&Asset{} and new(Asset) still leave ListensOnPorts nil, and api.go marshals the struct as-is. That means the BlueFlow null rejection can come back anywhere a caller skips this constructor. Please enforce the empty-slice invariant during marshal or immediately before upload, not only via constructor convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asset.go` around lines 12 - 17, NewAsset() alone isn't enough because callers
can create &Asset{} leaving ListensOnPorts nil; add enforcement at marshal time
by implementing Asset.MarshalJSON (or a prepare-for-upload helper called before
UploadAsset) to ensure ListensOnPorts is serialized as [] rather than null:
detect nil ListensOnPorts inside the custom MarshalJSON (or PrepareForUpload)
for type Asset and replace it with an empty slice before delegating to
json.Marshal so any direct &Asset{} or new(Asset) will still produce
"open_ports_tcp": [].
2 participants
Summary
-httpverbCLI flag (defaultPUT, allowsPOST) to configure the HTTP method for the BlueFlow upsert endpointipv4_address→ip_address,identifier→nameopen_port_tcp(string) →open_ports_tcp(int slice) to match BlueFlow serializerint-to-stringbuild error inhl7_decode_test.goand add Go modulesNewAsset()constructor to initializeopen_ports_tcpas[]instead ofnullin JSON — BlueFlow'sListFieldrejects nullIntegration test results
References
Test plan
-httpverbvalues are rejectedSummary by CodeRabbit
New Features
-httpverbflag (PUT by default, POST for legacy)Improvements
Chores