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.

Sign up for GitHub

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

Skip conventional sqlite:// URLs from safeguards #715

Merged
merged 1 commit into from
Jul 12, 2024
+22 −3
Merged

Skip conventional sqlite:// URLs from safeguards #715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/database_cleaner/safeguard.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require "uri"
Copy link
Contributor Author

@timriley timriley Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added this because running rspec spec/database_cleaner/safeguard_spec.rb without this require would raise a NameError.

Since there is no other require "uri" across the codebase, I figured that Safeguard was only working out in the world incidentally, by virtue of the apps requiring database_cleaner having already required "uri" before the code in this file was run.

I figured it would be good to add the require here given we have a clear dependency on "uri" within this file.


module DatabaseCleaner
class Safeguard
class Error < Exception
Expand Down Expand Up @@ -42,7 +44,8 @@ class RemoteDatabaseUrl
LOCAL = %w(localhost 127.0.0.1)

def run
raise Error::RemoteDatabaseUrl if !skip? && given?
return if skip?
raise Error::RemoteDatabaseUrl if given?
Comment on lines +47 to +48
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split this across two lines to match the other check classes in this file. It also helped me understand the second line a lot more easily.

end

private
Expand All @@ -54,7 +57,7 @@ def given?
def remote?(url)
return false unless url
parsed = URI.parse(url)
return false if parsed.scheme == 'sqlite3:'
return false if parsed.scheme == 'sqlite' || parsed.scheme == 'sqlite3'

host = parsed.host
return false if host.nil? || host.empty?
Expand Down
18 changes: 17 additions & 1 deletion spec/database_cleaner/safeguard_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,23 @@ module DatabaseCleaner
end
end

describe 'to a sqlite db' do
describe 'to a sqlite url' do
let(:database_url) { 'sqlite://tmp/db.sqlite3' }

it 'does not raise' do
expect { cleaner.start }.to_not raise_error
end
end

describe 'to a sqlite3 url' do
let(:database_url) { 'sqlite3://tmp/db.sqlite3' }

it 'does not raise' do
expect { cleaner.start }.to_not raise_error
end
end

describe 'to a sqlite3 url with no slashes after the scheme' do
let(:database_url) { 'sqlite3:tmp/db.sqlite3' }

it 'does not raise' do
Expand Down
Loading