Config db from url #269
Config db from url #269
Conversation
| @@ -256,12 +256,17 @@ module Marten | |||
| @csrf ||= GlobalSettings::CSRF.new | |||
| end | |||
|
|
|||
| # Allows to configure a specific database connection for the application. | |||
| def database(id = DB::Connection::DEFAULT_CONNECTION_NAME, &) | |||
| def database(id = DB::Connection::DEFAULT_CONNECTION_NAME, url : String | Nil = nil) | |||
There was a problem hiding this comment.
@ellmetha might be missing something with the overloading as it's my first time fiddling with crystal, but I think we have to stick with url as a named parameter here, therefore configuring with config.database url: "postgres://my_user:my_db@localhost:1234/db"
|
|
||
| params_map = uri.query_params.to_h | ||
|
|
||
| self.checkout_timeout = params_map.delete("checkout_timeout").try &.to_f64 || @checkout_timeout |
There was a problem hiding this comment.
Would you want to raise a specific exception here in case one of these casts fails?
|
Hi! Before reviewing could you kindly fix the failing GitHub QA check? This should be done after formatting |
| @@ -256,12 +256,17 @@ module Marten | |||
| @csrf ||= GlobalSettings::CSRF.new | |||
| end | |||
|
|
|||
| # Allows to configure a specific database connection for the application. | |||
There was a problem hiding this comment.
Can we keep method descriptions? 🙏
There was a problem hiding this comment.
Absolutely, accidental removal :)
| self.port = uri.port | ||
| self.user = uri.user | ||
| self.password = uri.password | ||
| self.name = uri.scheme == "sqlite" ? uri.host : uri.path[1..]? |
There was a problem hiding this comment.
It's worth noting that sqlite3 is a valid scheme for SQLite databases. We should definitely account for that!
|
|
||
| params_map = uri.query_params.to_h | ||
|
|
||
| self.checkout_timeout = params_map.delete("checkout_timeout").try &.to_f64 || @checkout_timeout |
There was a problem hiding this comment.
It would probably be safer to assign these values only when they are present in the query params:
| self.checkout_timeout = params_map.delete("checkout_timeout").try &.to_f64 || @checkout_timeout | |
| if params_map.has_key?("checkout_timeout") | |
| self.checkout_timeout = params_map.delete("checkout_timeout").try &.to_f64 | |
| end |
There was a problem hiding this comment.
Unless I'm missing something, the compiler does not like this assignment as it still thinks delete could return a Nil.
There was a problem hiding this comment.
I could also just deal with the deletion of these keys when setting self.options, and just read the value from the map inside the if clause, if that sounds better.
There was a problem hiding this comment.
Unless I'm missing something, the compiler does not like this assignment as it still thinks delete could return a Nil.
Yes, you have to check that you are in the presence a non-nil value. For example:
| self.checkout_timeout = params_map.delete("checkout_timeout").try &.to_f64 || @checkout_timeout | |
| if !(v = params_map.delete("checkout_timeout").try &.to_f64).nil? | |
| self.checkout_timeout = v | |
| end |
jpmartinspt
force-pushed
the
config-db-from-url
branch
from
d6411ef
to
7d2aae4
Compare
Add functionality so one can configure a database from a URL.
Fixes #257