Skip conventional sqlite:// URLs from safeguards

[timriley]

Update Safeguard::RemoteDatabaseUrl to skip URLs like "sqlite://path/to/file.sqlite", which is now the conventional format for URLs to SQLite databases when using libraries like Sequel.

Context

I'm currently preparing for the release of Hanami v2.2, which includes an integrated database layer for the first time in the 2.x series.

We are defaulting to an SQLite database, and are planning to use Database Cleaner in the test suite we generate for Hanami apps.

However, we were running afoul of your safeguard feature. This is because it considers our default database URL as non-local:

sqlite://db/app_name.sqlite

With this PR, we update Database Cleaner to permit these files, in keeping with some previous work to permit SQLite database URLs.

Background

The existing support for skipping SQLite URLs from tripping the Safeguard::RemoteDatabaseUrl check (implemented in #529) only worked for URLs in this exact format (note the missing slahes after the scheme):

sqlite3:path/to/file

And as Safeguard::RemoteDatabaseUrl evolved over time, the special handling of this kind of URL only continued to work incidentally. This early return would never trigger, because the scheme of the above URL is "sqlite3", not "sqlite3:" (note, no colon).

return false if parsed.scheme == 'sqlite3:'

Instead, the the Safeguard::RemoteDatabaseUrl would be skipped because further down, this would end up evaluating to true:

host = parsed.host
return false if host.nil?

For this particular URL string, its URI host is indeed nil.

Changes

The sqlite3:path/to/file format of URL is unusual in that it doesn't contain the typical double slashes after the scheme.

In current day usage of Ruby gems like Sequel, these slashes are expected, leading to URLs like this:

sqlite://path/to/file.sqlite

(This has in fact been Sequel's expected URL format since at least 2008)

To permit this, update the faulty scheme checking early return to return for your existing supported "sqlite3" URI scheme (sans the colon, so it actually works) as well as the conventional "sqlite".

return false if parsed.scheme == 'sqlite' || parsed.scheme == 'sqlite3'

Tests are update to match.

A humble request 😇

We're planning to release a Hanami 2.2 beta next week, with the full release coming about a month afterwards.

If you find this PR acceptable, I wonder if you might be able to turn around a quick patch release? It doesn't need to be ready for the beta, but if it could be done ~in the next month or so, that would be amazing.

This would allow us to generate Database Cleaner config without any extra ugliness. Currently we have to add the following to the top of our generated config:

# Allow Database Cleaner to work on our local sqlite databases
DatabaseCleaner.url_allowlist = [%r{^sqlite://}]

Thank you very much! ❤️

[timriley]

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.

[timriley]

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.

[etagwerker]

@timriley Looks good, thank you! ❤️

[timriley]

@etagwerker Thank you for taking care of this! (And hi! 😆)

It looks like there were some failing builds for this PR — sorry about that. I focused my own local testing on the unit tests only, but if there's something more I could do to help straighten things out, please let me know.

[timriley]

👋🏼 Hi again, @etagwerker! Just wanted to follow up on this one.

We're going to release Hanami 2.2 in the next few weeks, and we're going to include DatabaseCleaner in our default RSpec setup. I'd love it if we could avoid the ugly DatabaseCleaner.url_allowlist = [...] hack in all new Hanami apps. I think that'd cast both our projects in a poor light.

Might you be able to cut a new release in the next week or so?

I noted above that there's some failures on the main branch after this PR merged. If there's anything I can do to help address those, please let me know.

Thank you!

[etagwerker]

@timriley I just released v2.1.0 👍🏼

[timriley]

@etagwerker Thank you so much! 😍 I'll test everything again with some real Hanami 2.2 apps this weekend.