Skip to content

Al replace boltdb sqlite#313

Open
adalundhe wants to merge 5 commits into
decentralized-identity:mainfrom
adalundhe:AL-replace-boltdb-sqlite
Open

Al replace boltdb sqlite#313
adalundhe wants to merge 5 commits into
decentralized-identity:mainfrom
adalundhe:AL-replace-boltdb-sqlite

Conversation

@adalundhe
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@thereisnogabe thereisnogabe 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

}
switch u.Scheme {
case "bolt", "":
case "sqlite", "":
Copy link
Copy Markdown
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/sqlite"
)

type Storage interface {
Copy link
Copy Markdown
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