feat: Consolidate node options#4501
Conversation
…ty-optional-param
…ty-optional-param
…ty-optional-param
…-node-options # Conflicts: # .github/workflows/test-coverage.yml # cbindings/collection.go # cbindings/p2p.go # cbindings/query.go # cbindings/test_utils.go # cbindings/view.go # cbindings/wrapper.go # cbindings/wrapper_collection.go # cbindings/wrapper_tx.go # cli/collection.go # cli/collection_describe.go # cli/collection_get.go # cli/collection_truncate.go # cli/collection_update.go # cli/p2p_collection_create.go # cli/p2p_collection_delete.go # cli/p2p_document_create.go # cli/p2p_document_delete.go # cli/p2p_replicator_delete.go # cli/p2p_replicator_set.go # cli/request.go # client/collection.go # client/db.go # client/mocks/collection.go # client/mocks/txn.go # client/options/collection.go # client/options/identity.go # client/options/p2p.go # client/options/store.go # client/p2p.go # go.mod # go.sum # http/client.go # http/client_acp.go # http/client_collection.go # http/client_p2p.go # http/client_tx.go # http/handler_collection.go # http/handler_p2p.go # http/handler_store.go # internal/db/backup.go # internal/db/collection.go # internal/db/collection_delete.go # internal/db/collection_get.go # internal/db/collection_index.go # internal/db/collection_truncate.go # internal/db/collection_update.go # internal/db/db_dac.go # internal/db/db_nac.go # internal/db/merge.go # internal/db/p2p.go # internal/db/p2p/collection.go # internal/db/p2p/p2p.go # internal/db/store.go # internal/db/txn.go # internal/db/verify.go # internal/db/view.go # internal/planner/create.go # internal/planner/update.go # internal/planner/upsert.go # internal/request/graphql/schema/types/base.go # internal/se/coordinator.go # internal/se/mocks/db.go # js/client_collection.go # js/client_store.go # js/client_tx.go # js/js.go # tests/action/add_schema.go # tests/action/assert_request.go # tests/action/create_index.go # tests/action/create_view.go # tests/action/drop_index.go # tests/action/get_collections.go # tests/action/get_indexes.go # tests/action/refresh_view.go # tests/action/replace.go # tests/action/request.go # tests/action/truncate.go # tests/action/utils.go # tests/clients/cli/wrapper.go # tests/clients/cli/wrapper_acp.go # tests/clients/cli/wrapper_collection.go # tests/clients/cli/wrapper_tx.go # tests/clients/http/wrapper.go # tests/clients/http/wrapper_tx.go # tests/clients/js/wrapper.go # tests/clients/js/wrapper_collection.go # tests/clients/js/wrapper_tx.go # tests/integration/acp/nac/collection_get_by_id_test.go # tests/integration/acp/nac/collection_get_by_name_test.go # tests/integration/acp/nac/collection_get_by_version_test.go # tests/integration/acp/nac/collection_truncate_test.go # tests/integration/acp/nac/document_delete_test.go # tests/integration/acp/nac/document_update_with_filter_test.go # tests/integration/acp/nac/index_create_test.go # tests/integration/acp/nac/index_drop_test.go # tests/integration/acp/nac/index_list_test.go # tests/integration/acp/nac/p2p_collection_create_test.go # tests/integration/acp/nac/p2p_collection_delete_test.go # tests/integration/acp/nac/p2p_collection_list_test.go # tests/integration/acp/nac/p2p_document_create_test.go # tests/integration/acp/nac/p2p_document_delete_test.go # tests/integration/acp/nac/p2p_document_list_test.go # tests/integration/acp/nac/p2p_peer_connect_test.go # tests/integration/acp/nac/p2p_replicator_create_test.go # tests/integration/acp/nac/p2p_replicator_delete_test.go # tests/integration/acp/nac/relation_admin/collection_get_by_id_test.go # tests/integration/acp/nac/relation_admin/collection_get_by_name_test.go # tests/integration/acp/nac/relation_admin/collection_get_by_version_test.go # tests/integration/acp/nac/relation_admin/collection_truncate_test.go # tests/integration/acp/nac/relation_admin/index_create_test.go # tests/integration/acp/nac/relation_admin/index_drop_test.go # tests/integration/acp/nac/relation_admin/index_list_test.go # tests/integration/acp/nac/relation_admin/p2p_collection_create_test.go # tests/integration/acp/nac/relation_admin/p2p_collection_delete_test.go # tests/integration/acp/nac/relation_admin/p2p_collection_list_test.go # tests/integration/acp/nac/relation_admin/p2p_document_create_test.go # tests/integration/acp/nac/relation_admin/p2p_document_delete_test.go # tests/integration/acp/nac/relation_admin/p2p_document_list_test.go # tests/integration/acp/nac/relation_admin/p2p_peer_connect_test.go # tests/integration/acp/nac/relation_admin/p2p_replicator_create_test.go # tests/integration/acp/nac/relation_admin/p2p_replicator_delete_test.go # tests/integration/acp/nac/relation_admin/signature_verify_test.go # tests/integration/acp_dac.go # tests/integration/acp_dac_setup.go # tests/integration/acp_nac.go # tests/integration/collection/truncate/add_test.go # tests/integration/collection/truncate/dac_test.go # tests/integration/collection/truncate/index_filter_test.go # tests/integration/collection_version/updates/remove/view_test.go # tests/integration/db_setup.go # tests/integration/db_setup_js.go # tests/integration/encryption/peer_share_test.go # tests/integration/explain/execute/type_join_test.go # tests/integration/index/json_test.go # tests/integration/index/query_with_index_only_filter_test.go # tests/integration/index/query_with_relation_filter_test.go # tests/integration/p2p.go # tests/integration/p2p_collection.go # tests/integration/p2p_document.go # tests/integration/p2p_replicator.go # tests/integration/query/inline_array/with_filter_eq_test.go # tests/integration/query/json/with_geq_test.go # tests/integration/query/simple/with_cid_doc_id_test.go # tests/integration/subscription/with_commit_test.go # tests/integration/test_case.go # tests/integration/utils.go # tests/integration/view/simple/materialized_test.go # tests/state/state.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4501 +/- ##
===========================================
- Coverage 76.11% 76.10% -0.00%
===========================================
Files 519 521 +2
Lines 41728 41866 +138
===========================================
+ Hits 31758 31861 +103
- Misses 7489 7527 +38
+ Partials 2481 2478 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes node configuration by replacing scattered functional options with a fluent public builder API ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant OptionsBuilder as NodeOptionsBuilder
participant Node
participant Store
participant DB
participant P2P
participant HTTP
CLI->>OptionsBuilder: Node().Store().DB().P2P().HTTP().Set... (build options)
OptionsBuilder->>Node: node.New(ctx, opts)
Node->>Store: NewStore(ctx, opts.Store)
Store-->>Node: rootstore
Node->>DB: NewDB(ctx, rootstore, nodeACP, opts.DB)
DB-->>Node: db instance
Node->>P2P: startP2P(ctx, store, opts.P2P)
P2P-->>Node: peer
Node->>HTTP: NewServer(handler, opts.HTTP)
HTTP-->>Node: http server
Node->>Node: Start services (DB, P2P, HTTP)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🤖 Fix all issues with AI agents
In `@client/options/node.go`:
- Around line 215-248: The navigation methods on nodeSubBuilder (Node(),
Store(), DB(), P2P(), HTTP(), DocumentACP(), NodeACP()) dereference l.parent
without a nil check causing panic for standalone sub-builders; add a nil-check
at the start of each of these methods and if l.parent is nil, panic with a clear
descriptive message (e.g. "nodeSubBuilder: parent is nil; use NodeOptionsBuilder
or the parent constructor") so consumers get a helpful error instead of an
unexpected nil-pointer panic; update the seven methods in nodeSubBuilder to
perform this check before returning l.parent or delegating to parent
sub-builders.
In `@node/node_p2p.go`:
- Around line 39-47: The current conditional only appends p2p.WithEnablePubSub
when n.opts.P2P.EnablePubSub is true, which fails to disable PubSub because the
p2p library defaults it to true; update the node_p2p.go logic to always append
p2p.WithEnablePubSub with the explicit boolean value from
n.opts.P2P.EnablePubSub (i.e., call
p2p.WithEnablePubSub(n.opts.P2P.EnablePubSub)) so both true and false are
respected; keep the existing patterns for EnableRelay and
EnableClearBackoffOnRetry unchanged.
🧹 Nitpick comments (9)
node/acp_nac.go (1)
20-21: Consider guarding against nilopts.If
optsisnil, accessingopts.Pathandopts.IsEnabledwill panic. If callers always guarantee a non-nil pointer, a brief doc comment on the contract would be sufficient; otherwise, a nil check is warranted.node/acp_dac.go (1)
30-36: Nit: use_for the unusedaparameter.Line 32 names the
*options.NodeDocumentACPOptionsparameterabut never uses it. Convention is_for unused parameters.Proposed fix
options.NodeNoDocumentACPType: func( ctx context.Context, - a *options.NodeDocumentACPOptions, + _ *options.NodeDocumentACPOptions, ) (immutable.Option[dac.DocumentACP], error) { return dac.NoDocumentACP, nil },node/store_level.go (1)
29-30: Add a named constant for the"level"store type.Lines 29-30 use
options.NodeStoreType("level")as a raw string cast, while other store types (badger, memory) have named constants inclient/options/node.go. This pattern should be consistent: defineNodeLevelStore NodeStoreType = "level"in the constants block (lines 150-157) and use it throughout the codebase (node/store_level.go, tests/integration/db_setup.go, js/js.go).tests/integration/db_setup_js.go (1)
45-61: LGTM: DocumentACP configuration correctly uses builder pattern.The
SetAll(*s.DocumentACPOptions)call afterSetType(...)ensures all SourceHub-specific fields are applied. The ordering is correct sinceSetAllwill overwrite the type set bySetType, butDocumentACPOptionsalready contains the correct type (set insetupSourceHub).However, note that
SetType(options.NodeSourceHubDocumentACPType)on line 56 is redundant — the*s.DocumentACPOptionsreturned fromsetupSourceHubalready hasDocumentACPType: options.NodeSourceHubDocumentACPType(seeacp_dac_setup.goline 145). SinceSetAllon line 57 overwrites the entire struct, theSetTypecall on line 56 has no effect.♻️ Remove the redundant SetType call
opts.DocumentACP(). - SetType(options.NodeSourceHubDocumentACPType). SetAll(*s.DocumentACPOptions)internal/utils/util.go (1)
38-58: Silently swallowed errors fromNext()andValue()could mask builder bugs.Both
opt.Next()andopt.Value()can return errors, but they're silently consumed by breaking out of the loop. If a builder has a bug that causes enumeration errors, this would result in partial option application with no diagnostic output, making issues very difficult to debug.Consider logging at debug level when an error occurs, or at minimum adding a comment explaining why errors are intentionally swallowed.
♻️ Proposed: add debug-level logging or at least document the decision
func ApplyOptions[T any](target *T, opts ...enumerable.Enumerable[func(*T)]) { for _, opt := range opts { if opt == nil || reflect.ValueOf(opt).IsNil() { continue } for { hasNext, err := opt.Next() - if err != nil || !hasNext { + if err != nil { + // Errors during enumeration are not expected in normal operation + // and indicate a bug in the option builder implementation. + break + } + if !hasNext { break } setArgs, err := opt.Value() if err != nil { + // Same as above - unexpected in normal operation. break }internal/options/db.go (1)
53-63:SetNodeDBOptionsfollowed bySetAllin sequence could silently discard internal fields.
SetNodeDBOptions(line 54) replaces only the embeddedNodeDBOptions, preservingDocumentACPandP2P. ButSetAll(line 60) replaces the entireDBOptionsstruct (*o = opts), which would overwrite any previously setDocumentACPorP2P. Callers need to be aware thatSetAllis a full replacement, not a merge.In
node/node.goline 139,SetNodeDBOptionsis called first, thenSetDocumentACPandSetP2Pare called after — so the ordering is correct there. Just flagging this for awareness in case future callers chainSetAllafter setting internal fields.Consider adding a doc comment on
SetAllexplicitly warning that it replaces all fields including internal ones:-// SetAll sets all DB options from a plain data struct. +// SetAll replaces all DB options (including internal DocumentACP and P2P fields) +// from a plain data struct. Any previously set values will be overwritten. func (b *DBOptionsBuilder) SetAll(opts DBOptions) *DBOptionsBuilder {js/js.go (1)
45-54: Consider defining aNodeLevelStoreconstant for the "level" store type.Line 48 uses
options.NodeStoreType("level")as a raw string cast, while other store types (NodeBadgerStore,NodeMemoryStore) have dedicated constants inclient/options/node.go. The same raw cast appears intests/integration/db_setup.go(Line 122). Adding aNodeLevelStoreconstant would keep the pattern consistent and prevent typo-related bugs.Proposed constant in client/options/node.go
const ( NodeDefaultStore NodeStoreType = "" NodeBadgerStore NodeStoreType = "badger" NodeMemoryStore NodeStoreType = "memory" + NodeLevelStore NodeStoreType = "level" )Then in
js/js.go:- opts.Store().SetType(options.NodeStoreType("level")) + opts.Store().SetType(options.NodeLevelStore)http/server.go (1)
70-74: Zero-value HTTP timeouts mean "no timeout" by default.
ReadTimeout,WriteTimeout, andIdleTimeoutdefault to0(no timeout) when not explicitly set via options. This is likely pre-existing behavior, but it leaves the server vulnerable to slow-client attacks (e.g., Slowloris). Consider setting sensible defaults either here or inDefaultNodeOptions().client/options/node.go (1)
405-413:SetRetryIntervalssilently ignores empty slices, unlike other setters.The
if len(intervals) > 0guard inside the closure means callingSetRetryIntervals([]time.Duration{})is a no-op rather than clearing the intervals. All other setters unconditionally apply the value. If this guard is intentional (to preserve defaults), consider documenting it; otherwise, remove it for consistency.Remove the guard for consistency
func (sb *NodeDBOptionsBuilder) SetRetryIntervals(intervals []time.Duration) *NodeDBOptionsBuilder { sb.append(func(opts *NodeDBOptions) { - if len(intervals) > 0 { - opts.RetryIntervals = intervals - } + opts.RetryIntervals = intervals }) return sb }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/gen/cli/util_test.gois excluded by!**/gen/**tests/gen/schema_parser.gois excluded by!**/gen/**
📒 Files selected for processing (51)
cbindings/node.gocli/server_dump.gocli/start.goclient/options/node.godocs/website/references/cli/defradb_start.mdhttp/handler_ccip_test.gohttp/server.gohttp/server_test.gointernal/db/config.gointernal/db/config_test.gointernal/db/db.gointernal/db/db_test.gointernal/db/lens_p2p_config.gointernal/db/lens_p2p_config_js.gointernal/options/db.gointernal/options/enumerable.gointernal/utils/util.gojs/js.gojs/utils.gokeyring/signer.gonode/acp_dac.gonode/acp_dac_local.gonode/acp_dac_source_hub.gonode/acp_dac_source_hub_js.gonode/acp_nac.gonode/config.gonode/config_test.gonode/errors.gonode/node.gonode/node_api.gonode/node_js_test.gonode/node_p2p.gonode/node_p2p_js.gonode/node_test.gonode/store.gonode/store_badger.gonode/store_badger_test.gonode/store_level.gonode/store_memory.gonode/store_test.gotests/integration/acp_dac_setup.gotests/integration/acp_dac_setup_js.gotests/integration/db.gotests/integration/db_setup.gotests/integration/db_setup_js.gotests/integration/lens.gotests/integration/p2p_config.gotests/integration/p2p_config_js.gotests/integration/test_case.gotests/integration/utils.gotests/state/state.go
💤 Files with no reviewable changes (2)
- node/config.go
- node/store_test.go
🧰 Additional context used
🧬 Code graph analysis (35)
tests/integration/test_case.go (1)
client/options/node.go (1)
NodeP2POptions(103-116)
node/acp_dac_local.go (3)
client/options/node.go (3)
NodeDocumentACPOptions(151-164)NodeLocalDocumentACPType(52-52)NodeDefaultDocumentACPType(50-50)acp/dac/dac.go (2)
DocumentACP(31-133)NoDocumentACP(27-27)acp/dac/local.go (1)
NewLocalDocumentACP(27-36)
node/store_level.go (1)
client/options/node.go (2)
NodeStoreOptions(137-148)NodeStoreType(32-32)
internal/db/lens_p2p_config.go (1)
client/p2p.go (1)
Host(165-214)
node/store_memory.go (1)
client/options/node.go (3)
NodeStoreOptions(137-148)NodeDefaultStore(36-36)NodeMemoryStore(40-40)
tests/integration/lens.go (1)
client/options/node.go (1)
NodeLensRuntimeType(58-58)
http/server_test.go (3)
http/server.go (1)
NewServer(53-82)client/options/node.go (1)
NodeHTTP(500-502)http/errors.go (1)
ErrNoListener(35-35)
http/handler_ccip_test.go (1)
internal/db/db.go (1)
NewDB(122-129)
tests/integration/p2p_config_js.go (3)
tests/integration/p2p_config.go (1)
RandomNetworkingConfig(21-29)tests/integration/test_case.go (1)
ConfigureNode(118-118)client/options/node.go (1)
NodeP2POptions(103-116)
node/node_p2p.go (4)
client/p2p.go (1)
P2P(26-127)internal/db/p2p/p2p.go (1)
P2P(103-139)crypto/keys.go (1)
PrivateKey(66-72)internal/datastore/multi.go (1)
P2PBlockstoreFrom(113-117)
tests/integration/p2p_config.go (3)
tests/integration/p2p_config_js.go (1)
RandomNetworkingConfig(17-21)tests/integration/test_case.go (1)
ConfigureNode(118-118)client/options/node.go (1)
NodeP2POptions(103-116)
tests/state/state.go (1)
client/options/node.go (2)
NodeP2POptions(103-116)NodeDocumentACPOptions(151-164)
internal/db/lens_p2p_config_js.go (1)
client/p2p.go (1)
Host(165-214)
tests/integration/db_setup.go (2)
client/options/node.go (1)
NodeOptionsBuilder(251-253)node/node.go (1)
New(105-112)
node/acp_nac.go (3)
acp/nac/nac.go (1)
NewNodeACP(30-37)client/options/node.go (1)
NodeACPOptions(167-172)internal/db/acp/nac.go (2)
NACInfo(45-61)NewNACInfo(64-81)
internal/db/db_test.go (1)
internal/db/db.go (1)
NewDB(122-129)
node/acp_dac_source_hub_js.go (1)
client/options/node.go (2)
NodeSourceHubDocumentACPType(54-54)NodeDocumentACPOptions(151-164)
node/store.go (4)
client/options/node.go (2)
NodeStoreType(32-32)NodeStoreOptions(137-148)client/options/lister.go (1)
Lister(24-24)internal/utils/util.go (1)
NewOptions(32-36)node/errors.go (1)
NewErrStoreTypeNotSupported(32-34)
tests/integration/db.go (2)
client/options/node.go (2)
NodeOptionsBuilder(251-253)Node(256-258)node/node.go (2)
Node(54-65)New(105-112)
tests/integration/acp_dac_setup_js.go (2)
tests/state/state.go (1)
State(221-330)client/options/node.go (1)
NodeDocumentACPOptions(151-164)
cbindings/node.go (1)
client/options/node.go (2)
Node(256-258)NodeWASMLensRuntime(65-65)
node/errors.go (2)
client/options/node.go (2)
NodeStoreType(32-32)NodeDocumentACPType(44-44)errors/errors.go (1)
NewKV(33-38)
cli/server_dump.go (2)
node/store.go (1)
NewStore(55-69)client/options/node.go (1)
NodeStore(335-337)
node/acp_dac_source_hub.go (3)
client/options/node.go (2)
NodeSourceHubDocumentACPType(54-54)NodeDocumentACPOptions(151-164)node/errors.go (1)
ErrSignerMissingForSourceHubACP(25-25)acp/dac/source_hub.go (1)
NewSourceHubACP(35-49)
keyring/signer.go (1)
client/options/node.go (1)
NodeTxSigner(71-74)
http/server.go (5)
client/options/node.go (1)
NodeHTTPOptions(119-134)http/handler.go (1)
Handler(78-81)client/options/lister.go (1)
Lister(24-24)internal/utils/util.go (1)
ApplyOptions(39-59)http/middleware.go (1)
CorsMiddleware(33-45)
node/node_test.go (2)
node/node.go (2)
New(105-112)Node(54-65)client/options/node.go (1)
Node(256-258)
internal/db/db.go (7)
internal/options/enumerable.go (1)
Enumerable(19-19)internal/options/db.go (2)
DBOptions(22-29)DB(37-39)internal/utils/util.go (1)
ApplyOptions(39-59)internal/request/graphql/parser.go (1)
NewParser(40-52)internal/db/lock/lock.go (1)
NewLockSet(22-26)internal/db/config.go (1)
LensRuntimeType(47-47)internal/db/collection_retriever.go (1)
NewCollectionRetriever(30-34)
internal/db/config.go (2)
internal/options/db.go (1)
DBOptions(22-29)client/options/node.go (1)
NodeDBOptions(175-194)
tests/integration/acp_dac_setup.go (2)
tests/state/state.go (2)
State(221-330)DocumentACPType(72-72)client/options/node.go (3)
NodeDocumentACPOptions(151-164)NodeSourceHubDocumentACPType(54-54)NodeTxSigner(71-74)
internal/utils/util.go (1)
internal/options/enumerable.go (1)
Enumerable(19-19)
client/options/node.go (4)
tests/state/state.go (2)
KMSType(69-69)DocumentACPType(72-72)acp/dac/dac.go (1)
DocumentACP(31-133)internal/db/db.go (1)
DB(64-117)node/node.go (2)
DB(40-51)Node(54-65)
node/node.go (8)
client/options/node.go (9)
NodeOptions(78-100)NodeStoreOptions(137-148)NodeDocumentACPOptions(151-164)NodeACPOptions(167-172)NodeDBOptions(175-194)NodeP2POptions(103-116)NodeHTTPOptions(119-134)Node(256-258)NodeStore(335-337)acp/dac/dac.go (1)
DocumentACP(31-133)internal/db/db.go (1)
DB(64-117)internal/options/db.go (1)
DB(37-39)client/p2p.go (1)
P2P(26-127)internal/utils/util.go (1)
ApplyOptions(39-59)node/store.go (1)
NewStore(55-69)node/acp_nac.go (1)
NewNodeACP(20-22)
node/store_badger_test.go (3)
internal/utils/util.go (1)
NewOptions(32-36)client/options/node.go (1)
Node(256-258)node/node.go (1)
Node(54-65)
js/js.go (1)
client/options/node.go (3)
Node(256-258)NodeStoreType(32-32)NodeSourceHubDocumentACPType(54-54)
⏰ 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). (45)
- GitHub Check: Test macos job
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Check data format changes job
- GitHub Check: Test Limited Resource job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Start binary job
- GitHub Check: Check http documentation job
- GitHub Check: Validate containerfile job
- GitHub Check: Lint GoLang job
- GitHub Check: Check cli documentation job
- GitHub Check: Check wizard health job
🔇 Additional comments (63)
tests/integration/lens.go (1)
19-33: LGTM — clean migration to the consolidated options package.The type and import swap from
db.LensRuntimeTypetooptions.NodeLensRuntimeTypeis consistent with the PR's goal. Thedbimport is still needed fordb.InitContexton line 66.node/errors.go (1)
14-14: Clean type migration to consolidated options package — LGTM.The import addition and parameter type changes from local types to
options.NodeStoreTypeandoptions.NodeDocumentACPTypeare straightforward. Both arestringtypedefs, so they remain compatible witherrors.NewKV'svalue anyparameter. Function bodies are unchanged.Also applies to: 32-38
tests/integration/utils.go (3)
813-820: LGTM!Clean migration to the builder pattern. The node identity is correctly set using the current setup node ID before passing the builder to
setupNode.
835-845: LGTM!Consistent builder-based wiring of P2P options, node identity, and NAC enablement. The consolidated opts object is cleanly passed to
setupNode.
979-989: LGTM!Builder usage is consistent with the other call sites. The retry interval of 1ms is appropriate for test speed, and the P2P private key and identity are correctly wired before passing to
setupNode.docs/website/references/cli/defradb_start.md (1)
19-20: LGTM!Documentation update correctly narrows the
--developmentflag description to match the new identity-handling behavior.node/node_p2p_js.go (1)
21-21: LGTM!Clean migration from legacy config field to the new options-based
DisableP2Pflag.node/node_test.go (1)
28-34: LGTM!Clean adoption of the fluent builder pattern. The chaining through
.Store().SetPath(...).Node()correctly navigates the sub-builder and returns to the parent.http/server_test.go (1)
70-218: LGTM!Consistent migration of all test cases to the fluent
options.NodeHTTP()builder. Coverage of TLS, address binding, CORS origins, and timeout configuration remains intact.node/store_memory.go (1)
22-36: LGTM!Clean migration to
options.NodeStoreOptionsandoptions.NodeMemoryStore/options.NodeDefaultStoreconstants. The guard against overriding a previously-set default constructor is a nice defensive pattern.js/utils.go (1)
145-168: LGTM!The
initKeypairAndGetIdentityhelper is well-structured with proper error wrapping at each step. Key type (KeyTypeSecp256r1) is consistent with the existingcontextIdentityArgusage elsewhere in this file.node/config_test.go (1)
22-35: LGTM!Simple, focused tests that validate the builder-to-options pipeline for each boolean flag.
keyring/signer.go (1)
18-29: LGTM — interface assertion correctly updated tooptions.NodeTxSigner.The
txnSignertype implements bothGetAccAddress() stringandGetPrivateKey() cryptotypes.PrivKeyas required by the new interface definition inclient/options/node.go.node/store_level.go (1)
22-28: LGTM — constructor and purge functions correctly useoptions.NodeStoreOptions.The signatures and field access (
opts.Path) align with theNodeStoreOptionsstruct definition.node/acp_dac_local.go (1)
22-35: LGTM — local ACP constructor correctly migrated tooptions.NodeDocumentACPOptions.The constructor appropriately reads
opts.Path, and registering for bothNodeLocalDocumentACPTypeandNodeDefaultDocumentACPTypepreserves the prior behavior where local is the default ACP implementation.node/acp_dac_source_hub.go (1)
24-42: LGTM — SourceHub ACP constructor correctly migrated.The signer guard (
!opts.Signer.HasValue()) correctly prevents a panic before accessing.Value(). Field access for chain ID, gRPC address, and Comet RPC address aligns with theNodeDocumentACPOptionsstruct.node/store_badger_test.go (1)
24-41: LGTM — tests correctly exercise the new fluent builder API.Each test isolates a single store configuration option and verifies it through the builder chain. The pattern
options.Node().Store().Set*().Node()→utils.NewOptions()→ assert is clean and consistent.internal/db/config_test.go (1)
21-35: LGTM — tests cover the key defaults and mutability.Good coverage of the critical defaults (
MaxTxnRetries,EnableSigning,DocumentACP,P2P). The mutation test confirms the config struct works as expected withimmutable.Some.node/acp_dac.go (1)
39-48: LGTM —NewDocumentACPdispatch is clean and correct.The lookup-and-dispatch pattern is straightforward and the error path returns a proper typed
Nonewith a descriptive error.internal/db/config.go (1)
28-45: No action required.RetryIntervalsandMaxTxnRetriesserve independent purposes and are intentionally decoupled. The Coordinator generates its own retry intervals dynamically based onMaxTxnRetries(viadefaultRetryIntervals()), while P2P replication uses the configuredRetryIntervalsdirectly. The 7 intervals in the default config are not wasted—they support P2P replication independently of transaction retry limits.Likely an incorrect or invalid review comment.
node/store_badger.go (1)
24-63: LGTM!Clean migration of the badger store constructor and purge functions to the new
options.NodeStoreOptionsAPI. The conditional path handling, encryption index cache logic, and dual registration (badger + default) are all correctly preserved.node/store.go (1)
24-77: LGTM!The store constructor/purge registry and
NewStore/purgeStorefunctions are cleanly migrated to the centralizedoptions.NodeStoreOptionsAPI. Theutils.NewOptionspattern correctly builds the options object from the variadic listers.internal/db/db.go (1)
122-209: LGTM!The migration from mutable options slices to the config-driven
defaultDBConfig()+ApplyOptionspattern is clean. All DB fields are correctly populated from the resolved config, and the conditional P2P/lens wiring reads well. The separation ofNewDB(public) delegating tonewDB(testable) is a nice pattern to preserve.internal/db/db_test.go (1)
35-47: LGTM!Test call sites correctly updated to the simplified 3-argument API, consistent with the new variadic
optsparameter onNewDB/newDB.http/handler_ccip_test.go (1)
203-203: LGTM!Correctly aligned with the updated
NewDBsignature.node/acp_dac_source_hub_js.go (1)
24-31: LGTM!The JS build stub correctly migrates to the new
options.NodeDocumentACPOptionsparameter type while maintaining its simplified SourceHub ACP construction (opts fields intentionally unused in the JS variant).tests/integration/test_case.go (1)
118-118: LGTM!The
ConfigureNodetype signature cleanly migrated fromfunc() []node.Optiontofunc() options.NodeP2POptions, consistent with the centralized options pattern.tests/integration/acp_dac_setup_js.go (1)
18-19: LGTM!Clean migration to the new
*options.NodeDocumentACPOptionsreturn type, correctly delegating tos.DocumentACPOptions.node/node_api.go (1)
24-32: LGTM — Clean migration to the new options builder pattern.The switch from
n.config.disableAPIton.opts.DisableAPIand the use ofoptions.NodeHTTP().SetAll(n.opts.HTTP)to construct server options are consistent with the broader consolidation refactor.node/node_js_test.go (1)
28-34: LGTM — Test correctly exercises the fluent builder API.The chained builder pattern
options.Node().SetDisableP2P(true).SetDisableAPI(true).Store().SetBadgerInMemory(true).Node()is readable and provides good coverage of the new options surface for the JS build.internal/db/lens_p2p_config_js.go (1)
28-31: LGTM — Signature matches the non-JS build variant.The unused
client.Hostparameter (blank identifier) is appropriate here since JS builds don't support P2P, and both build variants must share the same function signature.tests/state/state.go (2)
203-204: LGTM —P2POptsvalue type is cleanly zero-initialized.Using
options.NodeP2POptionsas a value type (instead of the previous options slice) provides clear semantics and a safe zero value.
244-244: LGTM — Pointer-basedDocumentACPOptionsis appropriate.The change from a slice to
*options.NodeDocumentACPOptionscleanly supports the "no options" case vianil, and the removed initialization inNewStateis consistent sincenilis the zero value.cli/server_dump.go (1)
37-37: LGTM — Store creation migrated to builder pattern.
options.NodeStore().SetPath(badgerPath)is clean and consistent with theNewStoresignature accepting...options.Lister[options.NodeStoreOptions].node/node_p2p.go (1)
32-51: LGTM — P2P options construction is clean and readable.The conditional appending of P2P options based on
n.opts.P2Pfields is well-structured. Each option is only included when its value is meaningful (non-empty slices, non-zero booleans, non-empty key), and the blockstore is always included as a required option.internal/options/enumerable.go (1)
23-53: LGTM — Clean generic enumerable implementation for option builders.The
Next()/Value()cursor semantics are correctly implemented (advance-then-read pattern). Edge cases are handled well: empty builders returnfalseimmediately,Reset()enables re-iteration, andValue()beforeNext()safely returnsnil.internal/db/lens_p2p_config.go (1)
21-23: The nil guard concern does not apply. The function is only called whencfg.P2P.HasValue()is true (line 182 ofinternal/db/db.go), and only then iscfg.P2P.Value()passed toappendLensP2POpt. When P2P is unavailable, the else branch handles it by callinglensNode.WithP2PDisabled(true). Callers properly validate before passing the host.Likely an incorrect or invalid review comment.
tests/integration/db_setup_js.go (3)
31-35: LGTM: Clean nil-guard with default initialization.The nil check for
optswith fallback todefaultNodeOpts()is a good defensive pattern that ensures callers can passnilto get default behavior.
36-43: LGTM: Fluent builder configuration is clear and readable.The builder chain for DB and Store options is well-structured and easy to follow.
63-63: LGTM: Simplified node.New call.Passing the builder directly instead of spreading a slice is cleaner.
tests/integration/acp_dac_setup.go (1)
144-150: LGTM: Clean migration to the new options struct.The direct struct literal construction is clear and maps well to the new
NodeDocumentACPOptionsfields. The use ofimmutable.Some[options.NodeTxSigner](signer)for the optional signer is correct.tests/integration/db.go (3)
76-93: LGTM: Default options builder is well-structured.The fluent builder configuration is clean with good inline comments explaining the rationale for each setting.
113-131: LGTM: NewBadgerFileDB follows the same pattern as NewBadgerMemoryDB.Consistent use of the builder pattern with path-based storage configuration.
95-111: The builder chain implementation correctly handles the parent-child relationship. ThenodeSubBuilder[T]base type (whichNodeStoreOptionsBuilderembeds) has aNode()method that returns the parent*NodeOptionsBuilder, and theappend()method forwards sub-builder options back to the parent. The options set viaSetDisableP2P()andSetDisableAPI()are preserved throughout the chain. No changes needed.internal/utils/util.go (1)
32-36: LGTM: Clean decomposition into NewOptions + ApplyOptions.Extracting
ApplyOptionsas a separate function allows callers to apply options to an existing struct (as done innode.New) without requiring allocation of a new instance. Good separation of concerns.tests/integration/p2p_config_js.go (1)
17-29: LGTM: JS build stubs are consistent with the non-JS implementation signatures.The no-op stubs correctly match the function signatures in
p2p_config.go, maintaining build compatibility across platforms.node/node.go (4)
67-102: LGTM: Well-structured defaults with sensible values.The
DefaultNodeOptions()function provides a comprehensive set of defaults. The exponential retry intervals (30s → 32min) and 1GB Badger file size are reasonable production defaults. HavingEnableSigning: trueby default is a good security posture.
114-153: LGTM: Start() properly wires all subsystems from resolved options.The startup flow is clear: store → document ACP → node ACP → chunk size adjustment → P2P → DB → API. The conditional wiring of DocumentACP and P2P into the DB builder (lines 140-145) avoids unnecessary dependencies when those subsystems are disabled.
104-112: No type constraint issue.options.Lister[T]is a type alias forenumerable.Enumerable[func(*T)](line 24 ofclient/options/lister.go), so passingoptsof typeoptions.Lister[options.NodeOptions]toutils.ApplyOptions(which expectsenumerable.Enumerable[func(*options.NodeOptions)]) is valid and will compile without errors.
170-201: Store type remains constant acrossPurgeAndRestartcycles —ChunkSizehandling is safe.The
BadgerInMemoryflag, which determinesisValueSizeLimitedand thus whetherChunkSizeis set at line 131, is part ofn.opts.Store. Since neitherClose()norpurgeStore()modifiesn.opts, the store type is invariant across restart cycles. IfisValueSizeLimitedis true on the first cycle,ChunkSizewill be recomputed on each subsequent restart; if false, it is intentionally left unchanged. Existing tests confirm this behavior works correctly.internal/options/db.go (1)
22-29: LGTM: Clean extension of public options with internal-only fields.Embedding
publicOpts.NodeDBOptionsand addingDocumentACPandP2Pas optional internal fields is a good design — it keeps the public API surface clean while allowing internal components to pass additional context through the same options pipeline.tests/integration/p2p_config.go (2)
21-29: LGTM: Clean P2P options construction with sensible test defaults.The struct literal with
"/ip4/" + getIPString() + "/tcp/0"for listen address, relay enabled, and clear-backoff-on-retry enabled provides good defaults for integration testing.
54-60: LGTM: Simple and direct helper functions.Direct field assignment is preferable to the previous approach of building option slices. The function signatures match the JS build stubs in
p2p_config_js.go.tests/integration/db_setup.go (2)
56-61: LGTM — nil-guard and builder usage are clean.The nil check with
defaultNodeOpts()fallback at Lines 58-60 is a good defensive pattern, and the builder chaining throughout the function is consistent with the new API.
106-108: Verify cross-builder chaining navigates through the parent correctly.The chain
opts.Store().SetPath(path).DocumentACP().SetPath(path).NodeACP().SetPath(path)relies onnodeSubBuilder.DocumentACP()andnodeSubBuilder.NodeACP()delegating to the parent builder. This works per the implementation inclient/options/node.go(Lines 241-248), but since each.Store()/.DocumentACP()/.NodeACP()call creates a fresh ephemeral sub-builder, confirm that the forwarding viaparent.append(...)is the only mechanism relied upon (it is).http/server.go (1)
53-82: Clean migration to the options pattern.The
NewServerfunction properly defaults the address and applies options through the genericApplyOptionsutility. Thecfgis stack-allocated then escaped via&cfg— standard Go pattern.cli/start.go (2)
92-118: LGTM — builder-based configuration is well-structured and readable.The options are logically grouped by subsystem (Store, DB, P2P, HTTP, DocumentACP, NodeACP), making the configuration intent much clearer than the previous variadic slice approach.
416-458: Bounded recursion for legacy format migration looks correct.The recursive call on Line 445 only triggers when the old key format (no separator) is detected. After prepending the key type and writing back, the second call will find the separator and return normally. This is safe as written, though an iterative approach would be slightly more defensive.
cbindings/node.go (1)
41-93: LGTM — C bindings migration to builder pattern is clean.The conditional-only-when-set pattern preserves the previous behavior of falling back to defaults from
DefaultNodeOptions(). The WASM lens runtime is unconditionally set, which is appropriate for C binding builds.client/options/node.go (4)
196-213: Good design: dual-recording with parent forwarding.The
appendmethod records the option locally for standalone use and forwards to the parent when linked. Theparent != nilguard at Line 208 correctly handles both standalone and linked sub-builders.
292-326: Each sub-builder accessor allocates a new instance per call.Every call to
Store(),DB(),P2P(), etc. onNodeOptionsBuildercreates a new sub-builder. This is fine for the fluent chaining pattern, but worth noting that storing and reusing a sub-builder reference (e.g.,store := opts.Store()) works correctly becauseappendforwards to the parent. The ephemeral allocation is negligible since this runs at init time.
76-100: Well-structured central configuration type.The
NodeOptionsstruct cleanly aggregates all subsystem configurations with clear field documentation. This is a significant improvement over the scattered option functions.
174-194: No action needed —EnableSigningdefault is correctly set totrue.The default value is correctly initialized in
DefaultNodeOptions()(node/node.go, line 86) anddefaultDBConfig()(internal/db/config.go, line 32), both explicitly settingEnableSigning: true. The comment accurately reflects the actual behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/events.go (1)
261-261:⚠️ Potential issue | 🟡 MinorSame format-verb-in-
failureMessagebug exists here.Line 261 has the same issue this PR just fixed on lines 297–298:
"doc index %d out of range"is passed asfailureMessage, so%dis printed literally anddocIndexends up as unformattedmsgAndArgs. Consider applying the samefmt.Sprintftreatment.Suggested fix
- require.Fail(s.T, "doc index %d out of range", docIndex) + require.Fail(s.T, fmt.Sprintf("doc index %d out of range", docIndex))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/events.go
⏰ 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). (48)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test Limited Resource job
- GitHub Check: Check http documentation job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Check data format changes job
- GitHub Check: Check cli documentation job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Test macos job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Validate containerfile job
- GitHub Check: Start binary job
- GitHub Check: Lint GoLang job
- GitHub Check: Build dependencies job
- GitHub Check: Check wizard health job
- GitHub Check: Check mocks job
🔇 Additional comments (1)
tests/integration/events.go (1)
297-298: LGTM — correct use offmt.Sprintffor the failure message.Good fix.
require.Fail's second parameter is the literalfailureMessage, so format verbs embedded there wouldn't be interpolated. Wrapping withfmt.Sprintfis the right approach.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/options/node.go`:
- Around line 342-351: The comments for the sub-builder types claim they "can be
used standalone or as part of a NodeOptionsBuilder chain" but only NodeStore()
and NodeHTTP() have standalone constructors; add missing standalone constructor
functions for the other builders (e.g., NodeDB(), NodeP2P(), NodeDocumentACP(),
NodeACP()) that return pointers to their respective builder types (mirroring
NodeStore()'s pattern), or alternatively update the type comments on
NodeDBBuilder, NodeP2PBuilder, NodeDocumentACPBuilder, and NodeACPBuilder to
remove the "can be used standalone" phrase; locate types named NodeDB, NodeP2P,
NodeDocumentACP, and NodeACP (and their builder types) and implement matching
constructor functions or adjust their comments for consistency with
NodeStoreOptionsBuilder/NodeStore() and NodeHTTP().
🧹 Nitpick comments (3)
client/options/node.go (3)
306-340: Each navigation call allocates a fresh sub-builder — consider documenting this.Every call to
Store(),DB(),P2P(), etc. onNodeOptionsBuildercreates a newnodeSubBuilderinstance. This is safe becauseappendforwards options to the parent immediately, but it means the returned sub-builder is ephemeral — callingb.Store()twice yields two independent objects. If a consumer stores a reference and later expects it to reflect options set via a secondb.Store()call, only the parent's accumulated list will be correct, not the sub-builder's local list.This is a reasonable design, but a short doc comment on
NodeOptionsBuilder(or the accessor methods) noting this "each call returns a new sub-builder" behavior would help avoid confusion.
419-427: Inconsistent nil/empty-slice guard inSetRetryIntervals.
SetRetryIntervalssilently skips the assignment whenintervalsis empty, but analogous slice setters (SetListenAddresses,SetBootstrapPeers,SetAllowedOrigins,SetPrivateKey) unconditionally assign, including empty/nil slices. This inconsistency can confuse callers who expectSetRetryIntervals(nil)to clear the field.Either apply the guard consistently across all slice setters, or remove it here so the behavior is uniform.
Option: remove the guard for consistency
func (sb *NodeDBOptionsBuilder) SetRetryIntervals(intervals []time.Duration) *NodeDBOptionsBuilder { sb.append(func(opts *NodeDBOptions) { - if len(intervals) > 0 { - opts.RetryIntervals = intervals - } + opts.RetryIntervals = intervals }) return sb }
383-387:SetAlloverwrites all fields — worth a doc note.Each
SetAllmethod does*opts = newValue, which replaces the entire sub-config struct. If a caller chains individual setters beforeSetAll, those earlier values are silently discarded. This is likely intentional, but a brief godoc note (e.g., "SetAll replaces all fields; previously set values on this sub-builder are overwritten") would prevent misuse.Also applies to: 453-457, 501-505, 560-564, 608-612, 632-636
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/options/node.go
⏰ 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). (48)
- GitHub Check: Test macos job
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage JS job
- GitHub Check: Check http documentation job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage secondary index job
- GitHub Check: Test coverage encryption job
- GitHub Check: Check mocks job
- GitHub Check: Check cli documentation job
- GitHub Check: Test Limited Resource job
- GitHub Check: Check vulnerabilities job
- GitHub Check: Check data format changes job
- GitHub Check: Validate containerfile job
- GitHub Check: Check wizard health job
- GitHub Check: Start binary job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Lint GoLang job
- GitHub Check: Build dependencies job
🔇 Additional comments (1)
client/options/node.go (1)
76-100: Well-structured top-level config with clear field documentation.The
NodeOptionsstruct is clean and its inline comments make the purpose of each field immediately discoverable.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@client/options/node.go`:
- Around line 424-432: The SetRetryIntervals method currently skips assigning
when the incoming intervals slice is nil/empty, breaking consistency with other
setters like SetListenAddresses, SetBootstrapPeers, and SetAllowedOrigins and
preventing callers from clearing previously-set intervals; update the
NodeDBOptionsBuilder.SetRetryIntervals function to unconditionally assign
opts.RetryIntervals = intervals inside the append closure (so nil/empty will
clear the field), or if the empty-guard is required for a downstream invariant,
explicitly document that behavior on SetRetryIntervals and add a comment
explaining why it differs from the other slice setters.
- Around line 180-181: The EnableSigning field in NodeDBOptions should be made
explicitly optional to match its "Defaults to true" doc and other optional
fields: change the type from bool to immutable.Option[bool] on the NodeDBOptions
struct (symbol: EnableSigning), update all construction sites such as where
NodeDBOptions is initialized (node/node.go and internal/db/config.go) to set
immutable.Some(true) as the default, and update every usage site to read the
value via the Option accessor (e.g., .GetOrElse(true) / .ValueOr(true) or your
project's Option API) so direct struct literals like NodeDBOptions{} preserve
the documented default behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/options/node.gotests/integration/p2p_config.go
🧰 Additional context used
🧬 Code graph analysis (2)
client/options/node.go (1)
tests/state/state.go (2)
KMSType(69-69)DocumentACPType(72-72)
tests/integration/p2p_config.go (3)
tests/integration/p2p_config_js.go (1)
RandomNetworkingConfig(17-21)tests/integration/test_case.go (1)
ConfigureNode(118-118)client/options/node.go (1)
NodeP2POptions(103-116)
⏰ 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). (40)
- GitHub Check: Test macos job
- GitHub Check: Test Limited Resource job
- GitHub Check: Test coverage job (go, memory, gql)
- GitHub Check: Test coverage job (cli, memory, gql)
- GitHub Check: Test coverage job (cli, file, gql)
- GitHub Check: Test coverage document acp job (cli, source-hub)
- GitHub Check: Test coverage job (c, memory, collection-named)
- GitHub Check: Test coverage job (c, file, collection-named)
- GitHub Check: Test coverage job (http, file, collection-save)
- GitHub Check: Test coverage job (go, memory, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-named)
- GitHub Check: Test coverage job (cli, file, collection-save)
- GitHub Check: Test coverage job (http, memory, collection-named)
- GitHub Check: Test coverage job (cli, memory, collection-named)
- GitHub Check: Test coverage job (c, file, collection-save)
- GitHub Check: Test coverage job (go, file, collection-named)
- GitHub Check: Test coverage job (c, file, gql)
- GitHub Check: Test coverage job (http, file, collection-named)
- GitHub Check: Test coverage job (http, memory, gql)
- GitHub Check: Test coverage job (c, memory, collection-save)
- GitHub Check: Test coverage job (http, file, gql)
- GitHub Check: Test coverage job (go, memory, collection-save)
- GitHub Check: Test coverage job (c, memory, gql)
- GitHub Check: Test coverage job (http, memory, collection-save)
- GitHub Check: Test coverage job (cli, memory, collection-save)
- GitHub Check: Test coverage job (go, file, collection-save)
- GitHub Check: Test coverage job (go, file, gql)
- GitHub Check: Test coverage leveldb job
- GitHub Check: Test coverage lens job (wazero)
- GitHub Check: Test coverage JS job
- GitHub Check: Test coverage encryption job
- GitHub Check: Test coverage document acp job (http, source-hub)
- GitHub Check: Test coverage view job
- GitHub Check: Test coverage document acp job (go, source-hub)
- GitHub Check: Test coverage document acp job (c, source-hub)
- GitHub Check: Test coverage telemetry job
- GitHub Check: Check data format changes job
- GitHub Check: Validate containerfile job
- GitHub Check: Test NPX/JS build job
- GitHub Check: Lint GoLang job
🔇 Additional comments (4)
tests/integration/p2p_config.go (2)
18-28: Clean migration to the consolidated options struct.The import, return type, and struct literal fields all align with the new
options.NodeP2POptionsdefinition and theConfigureNodetype alias. Straightforward and correct.
55-61: Helpers are consistent with pointer-receiver mutation pattern.Both
withPrivateKeyandwithListenAddressescorrectly accept*options.NodeP2POptionsand directly set the respective fields. This is cleaner than the prior slice-based approach.client/options/node.go (2)
215-262:mustParent()guard looks good — previous concern addressed.The nil-check with a descriptive panic message is a clean solution for the standalone sub-builder safety concern.
1-22: Well-structured public API surface with clean builder pattern.The type hierarchy, constants, fluent builders, and standalone constructors form a cohesive and discoverable configuration API. The generic
nodeSubBuilder[T]withprojectfunction is an elegant way to avoid repetition across sub-builders.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // SetRetryIntervals sets the intervals between transaction retries. | ||
| func (sb *NodeDBOptionsBuilder) SetRetryIntervals(intervals []time.Duration) *NodeDBOptionsBuilder { | ||
| sb.append(func(opts *NodeDBOptions) { | ||
| if len(intervals) > 0 { | ||
| opts.RetryIntervals = intervals | ||
| } | ||
| }) | ||
| return sb | ||
| } |
There was a problem hiding this comment.
SetRetryIntervals silently ignores empty/nil input, unlike other slice setters.
SetListenAddresses, SetBootstrapPeers, and SetAllowedOrigins unconditionally assign the slice, but SetRetryIntervals silently no-ops on empty input. This asymmetry means callers cannot clear previously-set retry intervals via the builder.
If this guard is intentional (to protect a downstream invariant), consider documenting it. Otherwise, remove it for consistency:
Proposed fix
func (sb *NodeDBOptionsBuilder) SetRetryIntervals(intervals []time.Duration) *NodeDBOptionsBuilder {
sb.append(func(opts *NodeDBOptions) {
- if len(intervals) > 0 {
- opts.RetryIntervals = intervals
- }
+ opts.RetryIntervals = intervals
})
return sb
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SetRetryIntervals sets the intervals between transaction retries. | |
| func (sb *NodeDBOptionsBuilder) SetRetryIntervals(intervals []time.Duration) *NodeDBOptionsBuilder { | |
| sb.append(func(opts *NodeDBOptions) { | |
| if len(intervals) > 0 { | |
| opts.RetryIntervals = intervals | |
| } | |
| }) | |
| return sb | |
| } | |
| // SetRetryIntervals sets the intervals between transaction retries. | |
| func (sb *NodeDBOptionsBuilder) SetRetryIntervals(intervals []time.Duration) *NodeDBOptionsBuilder { | |
| sb.append(func(opts *NodeDBOptions) { | |
| opts.RetryIntervals = intervals | |
| }) | |
| return sb | |
| } |
🤖 Prompt for AI Agents
In `@client/options/node.go` around lines 424 - 432, The SetRetryIntervals method
currently skips assigning when the incoming intervals slice is nil/empty,
breaking consistency with other setters like SetListenAddresses,
SetBootstrapPeers, and SetAllowedOrigins and preventing callers from clearing
previously-set intervals; update the NodeDBOptionsBuilder.SetRetryIntervals
function to unconditionally assign opts.RetryIntervals = intervals inside the
append closure (so nil/empty will clear the field), or if the empty-guard is
required for a downstream invariant, explicitly document that behavior on
SetRetryIntervals and add a comment explaining why it differs from the other
slice setters.
nasdf
left a comment
nasdf
left a comment
There was a problem hiding this comment.
Changes look great! I left a comment with one question before you merge.
| chunkSize = immutable.Some(defaultChunkSize) | ||
|
|
||
| n.options = append(n.options, | ||
| db.WithLensOpts( |
There was a problem hiding this comment.
question: is lens chunk size still set correctly?
There was a problem hiding this comment.
the code that you pointing at seems to be outdated. This is how it looks now:
if isValueSizeLimited {
n.opts.DB.ChunkSize = immutable.Some(defaultChunkSize)
}Look correct to me. No?
2 participants
Relevant issue(s)
Resolves #4154
Description
Convert node options to follow new mongo-like options style.
Tasks
How has this been tested?
Integration tests
Specify the platform(s) on which this was tested: