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

Support readonly option in SQLite3Adapter #33242

Merged
merged 1 commit into from Jul 2, 2018

Conversation

Projects
None yet
3 participants
@brasic
Copy link
Contributor

brasic commented Jun 28, 2018

Summary

Readonly sqlite database files are very useful as a data format for storing configuration/lookup data that is too complicated for YAML files. But since such files would typically be committed to a source
control repository, it's important to ensure that they are truly safe from being inadvertently modified. Unfortunately using unix permissions isn't enough, as sqlite will "helpfully" add the write bit to a database file whenever it's written to.

sqlite3-ruby has supported a :readonly option since version 1.3.2 (see sparklemotion/sqlite3-ruby@c20c9f5)

This simply passes that option through to the sqlite adapter if present in the config hash. I think this is best considered an adapter-specific option since no other supported database has an identical concept.

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Jun 28, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kamipo (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@brasic brasic force-pushed the brasic:sqlite-readonly branch Jun 28, 2018

@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Jun 30, 2018

How about passing config itself like mysql2 adapter?

def mysql2_connection(config)
config = config.symbolize_keys
config[:flags] ||= 0
if config[:flags].kind_of? Array
config[:flags].push "FOUND_ROWS".freeze
else
config[:flags] |= Mysql2::Client::FOUND_ROWS
end
client = Mysql2::Client.new(config)

@brasic

This comment has been minimized.

Copy link
Contributor Author

brasic commented Jul 2, 2018

@kamipo pushed your suggested change. As far as I can tell there are no keys recognized by SQLite3::Database#initialize that would conflict with the keys used by the rails db config hash:

https://github.com/sparklemotion/sqlite3-ruby/blob/b6862d8f2542ce795ac6374ab1b1ddd08af8fd4d/lib/sqlite3/database.rb#L65-L110

@kamipo

kamipo approved these changes Jul 2, 2018

Copy link
Member

kamipo left a comment

Can you squash your commits into one?

Support readonly option in SQLite3Adapter
Readonly sqlite database files are very useful as a data format for
storing configuration/lookup data that is too complicated for YAML
files.  But since such files would typically be committed to a source
control repository, it's important to ensure that they are truly safe
from being inadvertently modified.  Unfortunately using unix permissions
isn't enough, as sqlite will "helpfully" add the write bit to a database
file whenever it's written to.

sqlite3-ruby has supported a `:readonly` option since version 1.3.2 (see
sparklemotion/sqlite3-ruby@c20c9f5)

This simply passes that option through to the adapter if present in the
config hash.  I think this is best considered an adapter-specific option
since no other supported database has an identical concept.

@brasic brasic force-pushed the brasic:sqlite-readonly branch to bf1024d Jul 2, 2018

@brasic

This comment has been minimized.

Copy link
Contributor Author

brasic commented Jul 2, 2018

Done, thanks!

@kamipo kamipo merged commit bf1024d into rails:master Jul 2, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

kamipo added a commit that referenced this pull request Jul 2, 2018

Merge pull request #33242 from brasic/sqlite-readonly
Support readonly option in SQLite3Adapter
@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Jul 2, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.