Repository navigation
Conversation
ac04253 to
6ce46d5
Compare
Replace the db utils' own JDBC URL handling and scheme aliases with the shared helper from hanami-db (hanami/hanami-db#25). hanami-db is required only when resolving a database class, rather than at the top of utils/database.rb, since every app command loads that file, including in apps that do not use hanami-db (an optional dependency of hanami-cli).
Raise the existing (previously unused) UnsupportedDatabaseSchemeError for unsupported schemes, and a new InvalidDatabaseURLError for URLs that cannot be parsed, rather than a RuntimeError or URI::InvalidURIError. Neither includes the URL in its message, nor carries the URI error as its cause, since the URL may contain credentials.
hanami-db is an optional dependency, so hanami-cli's gemspec cannot require a minimum version of it. Apps upgrading hanami-cli while keeping an older hanami-db would otherwise hit a NameError for Hanami::DB::DatabaseURL when running `hanami db` commands. This matches the equivalent check in hanami's db provider.
The E2E app is generated with released gems, so it installs the latest hanami-db from RubyGems. When hanami-cli depends on unreleased hanami-db changes (such as Hanami::DB::DatabaseURL), every E2E job fails with hanami-cli's "requires hanami-db 3.1.0 or later" error. Copy the hanami-db line from hanami-cli's own Gemfile into the app's Gemfile, so the app is always tested against the same hanami-db as hanami-cli's specs. This follows any temporary branch pointer and needs no change once it is reverted.
Temporary, until hanami/hanami-db#25's changes are merged. Revert before merging.
6ce46d5 to
c8bee10
Compare
timriley
left a comment
timriley
left a comment
There was a problem hiding this comment.
Looking good on the whole! I've a couple of questions for you that might lead to you making some changes, though. Let me know what you think!
| } | ||
| }.freeze | ||
|
|
||
| # Matches a URL's scheme, including any JDBC subprotocol, e.g. "jdbc:sqlserver" |
There was a problem hiding this comment.
Docs on regexps are good. Thank you!
| raise Hanami::CLI::InvalidDatabaseURLError.new(scheme) unless parseable?(database_url) | ||
|
|
||
| raise Hanami::CLI::UnsupportedDatabaseSchemeError.new(scheme) |
There was a problem hiding this comment.
I wonder if these errors should actually be defined in hanami-db?
That would be in keeping with the spirit of pushing all common logic down into that gem. And if we're creating new things now, we may as well start them there?
| require "hanami-db" | ||
| return if defined?(Hanami::DB::DatabaseURL) |
There was a problem hiding this comment.
This kind of "minimum version check" issue has come up a little while I've been working through hanami-webconsole changes too.
So far I've decided that we should just avoid this extra kind of work, and instead just make it very clear in our docs that Hanami gems are all designed to work only at the same [major].[minor] version combo.
What do you think about sticking with that approach for now and removing these checks?
Our upgrade notes will certainly make this clear. And newly generated Gemfiles will ensure the same minimum version for all gems too.
Figuring out a standardised way of doing this could be something that we sort out when we build our extensions API.
Hanami gems are designed to work together only at the same major.minor version, which upgrade notes and newly generated Gemfiles make clear. Checking for a minimum hanami-db here adds work we're not doing anywhere else, so drop it, along with HanamiDBVersionError, for now. A standard way to handle this can come with the extensions API. This reverts "Raise a clear error when hanami-db is too old" and removes the CHANGELOG entry about requiring hanami-db 3.1, matching the same change in hanami. The database E2E script still uses hanami-cli's own hanami-db, since without it the generated app would get a NameError for Hanami::DB::DatabaseURL until hanami-db 3.1 is released.
Depends on hanami/hanami-db#26, which should be merged first. Part of hanami/hanami-db#25.
hanami dbcommands now use hanami-db's newHanami::DB::DatabaseURLto recognise database URLs, including JRuby's JDBC URLs. This replaces their own JDBC prefix handling and scheme aliases.Changes
Utils::Database.database_classresolves the database class fromDatabaseURL.adapter. The JDBC handling inDatabase#database_uriandSqlite#namenow uses the same helpers.UnsupportedDatabaseSchemeErrorrather than aRuntimeError. URLs that can't be parsed raise a newInvalidDatabaseURLErrorrather thanURI::InvalidURIError. Neither error includes the URL or carries the URI error as its cause, since the URL may contain credentials.DatabaseURL,hanami dbcommands raise a newHanamiDBVersionErrorexplaining how to upgrade. hanami-db is only required when a db command runs, since every app command loadsutils/database.rb.Before merging
Hanami::DB::DatabaseURLfor inspecting database URLs hanami-db#26main(temporarily atdatabase-url-helpersso CI runs against it)