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

Convert SQLite directory to URI #2922

Merged
merged 35 commits into from
Jul 3, 2023
Merged

Conversation

w84thesun
Copy link
Contributor

@w84thesun w84thesun commented Jun 27, 2023

Description

Closes #2753.

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@w84thesun w84thesun self-assigned this Jun 27, 2023
@w84thesun w84thesun added the code/chore Code maintenance improvements label Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #2922 (4dc278b) into main (206abe8) will increase coverage by 0.05%.
The diff coverage is 67.10%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2922      +/-   ##
==========================================
+ Coverage   63.99%   64.04%   +0.05%     
==========================================
  Files         451      451              
  Lines       24074    24108      +34     
==========================================
+ Hits        15407    15441      +34     
+ Misses       7707     7705       -2     
- Partials      960      962       +2     
Impacted Files Coverage Δ
internal/backends/sqlite/backend.go 0.00% <0.00%> (ø)
internal/backends/sqlite/metadata/registry.go 0.00% <0.00%> (ø)
internal/handlers/registry/registry.go 55.55% <ø> (ø)
internal/handlers/registry/sqlite.go 6.25% <0.00%> (ø)
internal/handlers/sqlite/sqlite.go 0.00% <0.00%> (ø)
internal/backends/sqlite/metadata/pool/pool.go 70.37% <72.13%> (+1.89%) ⬆️
ferretdb/ferretdb.go 83.33% <100.00%> (ø)
integration/setup/listener.go 68.78% <100.00%> (+1.17%) ⬆️
integration/setup/startup.go 68.75% <100.00%> (+0.80%) ⬆️
Flag Coverage Δ
integration 57.35% <7.89%> (-0.08%) ⬇️
mongodb 4.84% <5.26%> (+<0.01%) ⬆️
pg 57.28% <7.89%> (-0.08%) ⬇️
shard-1 41.72% <7.89%> (-0.06%) ⬇️
shard-2 42.26% <7.89%> (-0.06%) ⬇️
shard-3 46.07% <7.89%> (-0.06%) ⬇️
unit 24.23% <64.28%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@w84thesun w84thesun requested review from a team, rumyantseva, chilagrow and noisersup June 29, 2023 16:56
@w84thesun w84thesun marked this pull request as ready for review June 29, 2023 16:56
@w84thesun w84thesun requested review from AlekSi and a team as code owners June 29, 2023 16:56
@w84thesun w84thesun enabled auto-merge (squash) June 29, 2023 16:56
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Looks good, just some tiny comments

cmd/ferretdb/main.go Outdated Show resolved Hide resolved
internal/backends/sqlite/backend.go Outdated Show resolved Hide resolved
internal/backends/sqlite/backend.go Outdated Show resolved Hide resolved
internal/backends/sqlite/backend.go Outdated Show resolved Hide resolved
internal/backends/sqlite/backend_test.go Outdated Show resolved Hide resolved
@w84thesun w84thesun requested a review from chilagrow June 30, 2023 07:25
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Looks good, let's update unit test to pass.

chilagrow
chilagrow previously approved these changes Jun 30, 2023
noisersup
noisersup previously approved these changes Jun 30, 2023
Copy link
Member

@noisersup noisersup left a comment

Choose a reason for hiding this comment

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

Looks good to me!

internal/backends/sqlite/backend_test.go Outdated Show resolved Hide resolved
cmd/ferretdb/main.go Outdated Show resolved Hide resolved
internal/backends/sqlite/metadata/pool/pool.go Outdated Show resolved Hide resolved
if err != nil {
return nil, lazyerrors.Error(err)
}

p := &Pool{
dir: dir,
dir: uri.Path,
Copy link
Member

Choose a reason for hiding this comment

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

🛑 And after that, we lose query parameters that we should pass to openDB below, as well in GetOrCreate via databasePath and maybe somewhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saved uri instead of dir.

Copy link
Member

Choose a reason for hiding this comment

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

That does not change the fact that openDB accepts matched files (without query parameters) in one place and the database path (also without query parameters) in another.

internal/backends/sqlite/backend.go Outdated Show resolved Hide resolved
internal/backends/sqlite/backend_test.go Outdated Show resolved Hide resolved
@w84thesun w84thesun dismissed stale reviews from noisersup and chilagrow via 047985a June 30, 2023 15:29
chilagrow
chilagrow previously approved these changes Jul 3, 2023
@AlekSi AlekSi disabled auto-merge July 3, 2023 07:49
@AlekSi AlekSi merged commit 0b347ae into FerretDB:main Jul 3, 2023
@AlekSi AlekSi added this to the v1.5.0 milestone Jul 3, 2023
@w84thesun w84thesun deleted the issue-2753-sqlite-uri branch July 3, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/chore Code maintenance improvements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Convert SQLite URI to directory
4 participants