Skip to content

fix: Prevent DB.Connect from panicking with P2P disabled #5227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChrisBQu wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix: Prevent DB.Connect from panicking with P2P disabled #5227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21cb047
First pass
ChrisBQu Sep 2, 2026
fc11482
Delint
ChrisBQu Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/db/p2p.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ func (db *DB) Connect(
return err
}

if db.p2p == nil {
return ErrNoP2P
}
return db.p2p.Connect(ctx, addresses)
}

Expand Down
27 changes: 27 additions & 0 deletions internal/db/p2p_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright 2026 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package db

import (
"context"
"testing"

"github.com/stretchr/testify/require"
)

// TestDB_Connect_WithoutP2P_ReturnsError tests that an error will be returned if
// Conneect is called with P2P disabled.
func TestDB_Connect_WithoutP2P_ReturnsError(t *testing.T) {

@AndrewSisley AndrewSisley Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This should really be an integration test. I'm leaving this as a suggestion as the existing NewNode action looks like it doesn't yet allow full configuration of the node - if you want to put that effort in it would be great, but no worries if not.

@ChrisBQu ChrisBQu Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're spot on for why it wasn't done as an integration test. I don't mind putting in the work to allow for it, but it seemed inappropriate to do during a feature freeze bug bash. I'm happy to use a unit test for now, and come back to this with an integration test next cycle, if you are?

@AndrewSisley AndrewSisley Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inappropriate to do during a feature freeze bug bash

No production change should be needed, if options.DBOptions.P2P is None, db.p2p will be nil and you can test this - only the NewNode action should need to change (to expose the public config, which it should have been doing anyway)

db := &DB{} // P2P is nil

err := db.Connect(context.Background(), []string{"/ip4/0.0.0.0/tcp/9171/p2p/12D3KooWBogusPeerID"})
require.ErrorIs(t, err, ErrNoP2P)
}
Loading