chore: modernize, optimize, and document the RDAP library#49
Merged
Conversation
- Bump go directive to 1.25 and update module dependencies
(kingpin v2.4.0, httpmock v1.4.1, x/crypto v0.52.0)
- Replace interface{} with any throughout
- Use reflect.Pointer over the deprecated reflect.Ptr
- Adopt maps.Copy and slices.Contains from the standard library
- Use strings.Builder in DecodeData.String()
- Replace the Go CI workflow with a dedicated build workflow
… paths Resolve each struct type's decode plan (where every RDAP field lives, and where the DecodeData field lives) once per Go type and cache it, instead of re-running reflection and struct-tag parsing on every struct decode. decodeStruct binds the cached plan to the instance and resolves only the fields present in the JSON. Slim down DecodeData, which is allocated for every decoded struct: - values now references the parsed source map directly instead of copying it. - isKnown shares the plan's read-only set of field names instead of being rebuilt per instance. - notes and overrideKnownValue are allocated lazily (most decodes need neither) rather than eagerly. Together these cut decoding of a representative nested domain response by ~53% in time, ~63% in allocated bytes, and ~49% in allocation count. What remains is dominated by encoding/json parsing into map[string]any and the reflection that fills the structs. The original BUG: invariant checks are preserved; they now run once when a type's plan is built. Also optimize three string hot paths: - escapePath: fast-path the common no-escape case (returns the input with zero allocation) and pre-size the buffer otherwise. - Printer.cleanString: skip the rune-by-rune strings.Map scan when the input contains no control runes. - VCard.Tel/Fax: call VCardProperty.Values once per property instead of twice; Fax now uses slices.Contains for consistency with Tel. Add benchmark_test.go covering the decode path and string hot paths as regression guards.
Add doc comments to every previously-undocumented exported function across the package (Client.Do, the Error()/String() methods, the bootstrap sort.Interface sorters, Response.ToWhoisStyleResponse, and the sandbox/test helpers), and complete the comments for the Printer's print* routines. Also fix pre-existing gofmt drift in bootstrap/cache/memory_cache.go, bootstrap/question.go, and test/http_test.go.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPR migrates 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 204ab3fa-e565-4545-abcf-826605294d79
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.github/workflows/build.yml.github/workflows/go.yml.gitignorebenchmark_test.gobootstrap/answer.gobootstrap/asn_registry.gobootstrap/cache/memory_cache.gobootstrap/cache/registry_cache.gobootstrap/client.gobootstrap/net_registry.gobootstrap/question.goclient.goclient_error.godecode_data.godecoder.godecoder_test.gogo.modprint.gorequest.goresponse.gosandbox/file.gotest/file.gotest/http.gotest/http_test.govcard.govcard_test.go
💤 Files with no reviewable changes (1)
- .github/workflows/go.yml
Add golangci-lint config (.golangci.yml) tuned for this library, plus Lint and Tests GitHub Actions workflows with tightened token scopes. Resolve all linter findings, including genuine fixes: - propagate context via http.NewRequestWithContext (noctx) - fix &*r.Server aliasing that mutated the shared Server URL (SA4001) - lowercase error strings and drop redundant "Error" prefixes (ST1005) - add explicit json tags to the bootstrap unmarshal struct (musttag) - replace test init() with explicit var initialization - tighten cache dir/file permissions to 0750/0600 - drop always-nil error returns from the decodeX helpers
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28535d5a-c898-4bf3-ae53-24019730f0b1
📒 Files selected for processing (37)
.github/workflows/lint.yml.github/workflows/tests.yml.golangci.ymlbenchmark_test.gobootstrap/asn_registry.gobootstrap/asn_registry_test.gobootstrap/cache/disk_cache.gobootstrap/cache/disk_cache_test.gobootstrap/cache/memory_cache.gobootstrap/cache/memory_cache_test.gobootstrap/client.gobootstrap/client_test.gobootstrap/dns_registry.gobootstrap/dns_registry_test.gobootstrap/file.gobootstrap/file_test.gobootstrap/net_registry.gobootstrap/net_registry_test.gobootstrap/service_provider_registry.gobootstrap/service_provider_registry_test.gocli.goclient.goclient_error.goclient_test.godecoder.godecoder_test.goexample_test.goprint.goprint_test.gorequest.gorequest_test.gosandbox/file.gotest/file.gotest/http.gotest/http_test.govcard.govcard_test.go
💤 Files with no reviewable changes (2)
- bootstrap/client_test.go
- test/file.go
✅ Files skipped from review due to trivial changes (8)
- bootstrap/asn_registry_test.go
- bootstrap/dns_registry_test.go
- bootstrap/service_provider_registry_test.go
- bootstrap/net_registry_test.go
- request_test.go
- example_test.go
- bootstrap/cache/disk_cache_test.go
- sandbox/file.go
🚧 Files skipped from review as they are similar to previous changes (9)
- test/http_test.go
- vcard_test.go
- benchmark_test.go
- client.go
- bootstrap/cache/memory_cache.go
- request.go
- print.go
- vcard.go
- decoder.go
Drive RunCLI for each output mode (text/json/whois/raw) plus version and error paths, comparing exit code, stdout, and stderr against committed golden files. Network-backed cases use an in-process fixture server via --server and an in-memory cache, so nothing touches the network or the real filesystem. Regenerate with `go test -run TestRunCLI -update`. Also sort unknown-field output in the printer (printUnknowns and the nested-map case of printUnknown) so CLI output is deterministic. Raises RunCLI coverage from 0% to ~48%.
go.mod declares a 1.25.0 floor while CI ran only 1.26, which could mask bugs that surface on the declared minimum. Test on both the floor and the latest release; lint continues on the latest toolchain only.
Distribute the benchmarks from benchmark_test.go into the _test.go file of the package code each one exercises, matching this repo's per-file test layout (idiomatic Go; the standard library colocates benchmarks too): BenchmarkDecodeDomain -> decoder_test.go BenchmarkEscapePathClean/Dirty -> request_test.go BenchmarkCleanStringClean/Dirty -> print_test.go BenchmarkVCardValues -> vcard_test.go BenchmarkDecodeDomain now loads its fixture via test.LoadFile for consistency (outside the timed loop, so results are unaffected).
The uint->int conversion in Printer.indent for strings.Repeat cannot realistically overflow (indent depth is bounded by RDAP object nesting). An in-code //nolint kept getting stripped by editor tooling, so suppress it in .golangci.yml instead, which is stable across edits.
Member
Author
Benchmarks:
|
| Benchmark | master | branch | Δ |
|---|---|---|---|
| DecodeDomain | 62.62µ ± 1% | 29.95µ ± 7% | −52.16% (p=0.000) |
| EscapePathClean | 33.29n ± 2% | 8.99n ± 3% | −72.99% (p=0.000) |
| EscapePathDirty | 87.47n ± 2% | 79.94n ± 6% | −8.61% (p=0.000) |
| CleanStringClean | 293.1n ± 2% | 93.31n ± 1% | −68.16% (p=0.000) |
| CleanStringDirty | 74.70n ± 2% | 79.59n ± 2% | +6.54% (p=0.000) |
| VCardValues | 12.99n ± 5% | 12.99n ± 3% | ~ (p=0.670) |
| geomean | 193.1n | 113.0n | −41.51% |
Memory (B/op)
| Benchmark | master | branch | Δ |
|---|---|---|---|
| DecodeDomain | 65.01Ki ± 0% | 23.94Ki ± 0% | −63.18% (p=0.000) |
| EscapePathClean | 16.00 ± 0% | 0.00 ± 0% | −100.00% (p=0.000) |
| EscapePathDirty | 128.0 ± 0% | 160.0 ± 0% | +25.00% (p=0.000) |
| CleanStringClean | 0 | 0 | ~ |
| CleanStringDirty | 32.00 ± 0% | 32.00 ± 0% | ~ |
| VCardValues | 16.00 ± 0% | 16.00 ± 0% | ~ |
Allocations (allocs/op)
| Benchmark | master | branch | Δ |
|---|---|---|---|
| DecodeDomain | 966.0 ± 0% | 489.0 ± 0% | −49.38% (p=0.000) |
| EscapePathClean | 1.000 ± 0% | 0.000 ± 0% | −100.00% (p=0.000) |
| EscapePathDirty | 2.000 ± 0% | 2.000 ± 0% | ~ |
| CleanStringClean | 0 | 0 | ~ |
| CleanStringDirty | 1.000 ± 0% | 1.000 ± 0% | ~ |
| VCardValues | 1.000 ± 0% | 1.000 ± 0% | ~ |
Summary
- Headline: decoding a realistic domain response is ~2x faster (−52% time, −63% memory, −49% allocs) thanks to the cached field-plan.
- Common hot paths (
escapePath/cleanStringclean inputs) are ~3x faster and now zero-alloc. - Minor regressions on the uncommon "dirty" paths:
EscapePathDirtyuses +25% B/op (128→160) andCleanStringDirtyis +6.5% slower; both the rare escaping branches, a deliberate trade for the common-case wins. VCardValuesis unchanged (noise).
Raw benchstat output
goos: darwin
goarch: arm64
pkg: github.com/openrdap/rdap
cpu: Apple M3 Pro
│ /tmp/bench_master.txt │ /tmp/bench_branch.txt │
│ sec/op │ sec/op vs base │
DecodeDomain-12 62.62µ ± 1% 29.95µ ± 7% -52.16% (p=0.000 n=10)
EscapePathClean-12 33.290n ± 2% 8.993n ± 3% -72.99% (p=0.000 n=10)
EscapePathDirty-12 87.47n ± 2% 79.94n ± 6% -8.61% (p=0.000 n=10)
CleanStringClean-12 293.10n ± 2% 93.31n ± 1% -68.16% (p=0.000 n=10)
CleanStringDirty-12 74.70n ± 2% 79.59n ± 2% +6.54% (p=0.000 n=10)
VCardValues-12 12.99n ± 5% 12.99n ± 3% ~ (p=0.670 n=10)
geomean 193.1n 113.0n -41.51%
│ /tmp/bench_master.txt │ /tmp/bench_branch.txt │
│ B/op │ B/op vs base │
DecodeDomain-12 65.01Ki ± 0% 23.94Ki ± 0% -63.18% (p=0.000 n=10)
EscapePathClean-12 16.00 ± 0% 0.00 ± 0% -100.00% (p=0.000 n=10)
EscapePathDirty-12 128.0 ± 0% 160.0 ± 0% +25.00% (p=0.000 n=10)
CleanStringClean-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
CleanStringDirty-12 32.00 ± 0% 32.00 ± 0% ~ (p=1.000 n=10) ¹
VCardValues-12 16.00 ± 0% 16.00 ± 0% ~ (p=1.000 n=10) ¹
geomean ² ? ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean
│ /tmp/bench_master.txt │ /tmp/bench_branch.txt │
│ allocs/op │ allocs/op vs base │
DecodeDomain-12 966.0 ± 0% 489.0 ± 0% -49.38% (p=0.000 n=10)
EscapePathClean-12 1.000 ± 0% 0.000 ± 0% -100.00% (p=0.000 n=10)
EscapePathDirty-12 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹
CleanStringClean-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
CleanStringDirty-12 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
VCardValues-12 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
geomean ² ? ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean
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.
Summary
Modernizes the codebase, optimizes the decoder hot paths, and adds documentation across exported APIs.
DecodeData, and optimize string hot pathsgo.ymlwith abuild.ymlworkflow, expand.gitignorefor editor/IDE/OS artifactsgo fixfor modernizationChanges
benchmark_test.gocovering decode pathsdecoder.gowith cached field plans (largest change)print.goandrequest.gogo.mod/go.sumTest plan
go test ./...go vet ./...Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores