Skip to content
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.

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

Al replace boltdb sqlite #313

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

adalundhe
Copy link

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ New Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 👷 Example Application
  • 🧑‍💻 Code Snippet
  • 🎨 Design
  • 📖 Content
  • 🧪 Tests
  • 🔖 Release
  • 🚩 Other

Description

This PR removes BoltDB and replaces it with a sqlc-generated SQLite connection, as per #121, (see here. This includes removing BoltDB defaults and tests and replacing them with SQLite tests (largely based off of the PostgreSQL tests), as well as like connection logic an interface(s) (also likewise based off of the PostgreSQL code).

Related Tickets & Documents

Resolves #121

Mobile & Desktop Screenshots/Recordings

Added code snippets?

  • 👍 yes
  • 🙅 no, because they aren't needed

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

No tests? Add a note

Added to documentation?

  • 📜 readme
  • 📜 contributing.md
  • 📓 general documentation
  • 🙅 no documentation needed

No docs? Add a note

[optional] Are there any post-deployment tasks we need to perform?

N/A

[optional] What gif best describes this PR or how it makes you feel?

Anything happy and dancing!

@adalundhe
Copy link
Author

@blackgirlbytes BOOM! Happy Halloween! Got it done just in time!

@adalundhe adalundhe marked this pull request as ready for review October 31, 2024 19:10
@adalundhe
Copy link
Author

One quick note - I did notice the TestGetPutDIDDHT fails with the following output:

--- FAIL: TestGetPutDIDDHT (0.00s)
    dns_test.go:70:
                Error Trace:    /home/ada/Projects/did-dht/impl/pkg/dht/dns_test.go:70
                Error:          Received unexpected error:
                                failed to initialize *jwk.ecdsaPublicKey from *ecdsa.PublicKey: invalid elliptic curve &{%!s(*elliptic.CurveParams=&{0xc0001553e0 0xc000155420 0xc0001554e0 0xc000155460 0xc0001554a0 256 secp256k1})}

Is this expected, or am I missing a test config option/etc.?

@blackgirlbytes
Copy link
Contributor

Hey @frankhinek , @decentralgabe , @angiejones ..we just got this amazing PR to replace bolt with SQLite. Since today is the last day of Hacktoberfest, any of you mind reviewing this and then merging it so Ada can get credit? Thank you!

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

great start! please take a look at my comments

}

func (s SQLite) migrate() error {
db, err := sql.Open("sqlite3", string(s))
Copy link
Member

Choose a reason for hiding this comment

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

please make sqlite3 a const

defer db.Close()

goose.SetBaseFS(migrations)
if err = goose.SetDialect("sqlite"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

same with sqlite

return err
}

if err = goose.Up(db, "migrations"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

and migrations


err = db.write(ctx, namespace, "tezos-testnet", dummyData)
assert.NoError(t, err)
func getTestDB(t *testing.T) storage.Storage {
Copy link
Member

Choose a reason for hiding this comment

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

please add some tests for the db

type SQLite string

// NewSQLite creates a SQLite-based implementation of storage.Storage
func NewSQLite(uri string) (SQLite, error) {
Copy link
Member

Choose a reason for hiding this comment

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

check for empty str?

@@ -32,13 +32,14 @@ func NewStorage(uri string) (Storage, error) {
return nil, err
}
switch u.Scheme {
case "bolt", "":
case "sqlite", "":
Copy link
Member

Choose a reason for hiding this comment

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

can this be removed and just made the default?

"github.com/TBD54566975/did-dht/pkg/storage/db/postgres"
"github.com/TBD54566975/did-dht/pkg/storage/db/sqlite"
)

type Storage interface {
Copy link
Member

Choose a reason for hiding this comment

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

wondering if there should be a set of dynamic dispatch methods over the storage calls that can do a common set of validations (empty checks, for example)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace BoltDB with sqlite
3 participants