Skip to content

fix(sqlite): fix wrongly overwriting storage if empty string#13376

Merged
sdepold merged 4 commits into
sequelize:mainfrom
soryy708:fix-anon-disk-based-sqlite
Oct 17, 2021
Merged

fix(sqlite): fix wrongly overwriting storage if empty string#13376
sdepold merged 4 commits into
sequelize:mainfrom
soryy708:fix-anon-disk-based-sqlite

Conversation

@soryy708
Copy link
Copy Markdown
Contributor

@soryy708 soryy708 commented Jul 16, 2021 •
edited
Loading

Empty string in SQLite3 means anonymous disk-based db, but sequelize overwrited it with :memory:

Closes #13375

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Closes #13375

@soryy708
fix(sqlite): fix wrongly overwriting storage if empty string
939619e 
Empty string in SQLite3 means anonymous disk-based db, but sequelize overwrited it with :memory:

Closes sequelize#13375
@soryy708
Copy link
Copy Markdown
Contributor Author

soryy708 commented Jul 16, 2021

I get 3 failing tests, but they happen when running test-sqlite on main without my changes.

@sdepold sdepold self-assigned this Oct 16, 2021
sdepold
sdepold requested changes Oct 16, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sdepold sdepold left a comment

Choose a reason for hiding this comment

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

What is the diff between :memory: and anonymous disk-based database?

Comment thread lib/dialects/sqlite/connection-manager.js Outdated
@sdepold sdepold added the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 16, 2021
@soryy708
Copy link
Copy Markdown
Contributor Author

soryy708 commented Oct 16, 2021

What is the diff between :memory: and anonymous disk-based database?

:memory: means the database is anonymous and in-memory, so not persisted to the disk. anonymous disk-based database means the database is persisted to the disk, and the file name is controlled by sqlite.
SQLite documentation states:

filename: Valid values are filenames, ":memory:" for an anonymous in-memory database and an empty string for an anonymous disk-based database. Anonymous databases are not persisted and when closing the database handle, their contents are lost.

@github-actions github-actions Bot removed the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 16, 2021
@sdepold
Copy link
Copy Markdown
Member

sdepold commented Oct 17, 2021

So it's still a temporary database that gets deleted after stopping the app, correct? With the only difference that it is either memory or hard drive based.

@sdepold
Copy link
Copy Markdown
Member

sdepold commented Oct 17, 2021

Seems that all tests are green except for linking. Would you like to have a look and fix it?

@sdepold sdepold added the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 17, 2021
@soryy708
Copy link
Copy Markdown
Contributor Author

soryy708 commented Oct 17, 2021

So it's still a temporary database that gets deleted after stopping the app, correct? With the only difference that it is either memory or hard drive based.

The way I understand it, yes. The use case for a temporary on-disk database is temporary workloads on a memory-constrained machine, like a CI server.

Seems that all tests are green except for linking. Would you like to have a look and fix it?

I'm looking in to it. That's odd, because when I ran the linter locally it all passed.

@github-actions github-actions Bot removed the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 17, 2021
@soryy708
Copy link
Copy Markdown
Contributor Author

soryy708 commented Oct 17, 2021

@sdepold All checks passed now. Except 1 skipped check?

@sdepold
Copy link
Copy Markdown
Member

sdepold commented Oct 17, 2021

Nice one. I'm going to refactor the nullish coalescence refactoring myself in a follow up PR.

Thanks a bunch!

@sdepold sdepold merged commit c3e608b into sequelize:main Oct 17, 2021
@sdepold sdepold mentioned this pull request Oct 17, 2021
Merged
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 24, 2021

🎉 This PR is included in version 6.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
@soryy708 @sdepold @aliatsis
fix(sqlite): fix wrongly overwriting storage if empty string (sequeli…
c35dc57 
…ze#13376)

* fix(sqlite): fix wrongly overwriting storage if empty string

Empty string in SQLite3 means anonymous disk-based db, but sequelize overwrited it with :memory:

Closes sequelize#13375

* fix(sqlite): fix node<10 chokes on nullish coalescing operator

* refactor(sqlite): remove unnecessary parentheses around expression

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdepold sdepold sdepold approved these changes

Assignees

@sdepold sdepold

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sequelize doesn't respect storage parameter for SQLite3 anonymous disk-based database

2 participants

@soryy708 @sdepold