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

Meaningful error when using incorrect database url protocol #79

Closed
thangngoc89 opened this issue Jun 28, 2021 · 9 comments · Fixed by #92
Closed

Meaningful error when using incorrect database url protocol #79

thangngoc89 opened this issue Jun 28, 2021 · 9 comments · Fixed by #92
Milestone

Comments

@thangngoc89
Copy link
Contributor

thangngoc89 commented Jun 28, 2021

Update:

This error is because incorrect database protocol was used (sqlite instead of sqlite3)

——-
Instead of this

web   | 29.06.21 03:06:04.924      dream.http ERROR Failure("Dream.sql_pool: cannot create pool for 'sqlite:development.sqlite': Failed to load driver for <sqlite:devel)
web   | 29.06.21 03:06:04.924      dream.http ERROR Raised at Stdlib__map.Make.find in file "map.ml", line 137, characters 10-25
web   | 29.06.21 03:06:04.924      dream.http ERROR Called from Logs.Tag.find in file "src/logs.ml", line 154, characters 14-32

I believe this would be the expected behavior since all of sqlite3 tools I've used do this

@tungd
Copy link

tungd commented Jun 29, 2021

From the error message I guess you forgot to link the required driver on your build script, for example:

dune:

(executable
  (libraries dream caqti-driver-sqlite3))

Otherwise the db do get created automatically.

@thangngoc89
Copy link
Contributor Author

@tungd I added caqti-driver-sqlite3 to dune . This error resolved itself when I created a new sqlite3 database at the correct location. Hence this issue

@aantron
Copy link
Owner

aantron commented Jun 29, 2021

@thangngoc89 I agree that the database should be created automatically. However, in agreement with @tungd, I see that it is. For instance, I took the h-sql example and deleted db.sqlite. The example still ran, created db.sqlite, and then requests to the example server failed with error messages about missing tables, etc. (as expected). I've also only observed the error in your original comment when caqti-driver-sqlite3 was not linked, also in agreement with @tungd.

If you can reproduce the original error and confirm you were linking the driver, could you say what version of sqlite3 (on the system or in esy) is installed? What versions of ocaml-sqlite3 and caqti? Maybe if something old is installed, it's possible that error codes are being interpreted incorrectly somewhere along the way.

The creation of the database is kind of accidental at this point, because database init (and migrations) need a rework, and all of that might be best handled outside of Dream (though I do want to make sure it is done well, exists, and we can link to it for people to use it, and I don't mean that it is outside of the responsibility of Web library authors, just outside of the scope of Dream itself).

@thangngoc89
Copy link
Contributor Author

I can confirm that this is error on my part. I can’t reproduce the error above

@aantron
Copy link
Owner

aantron commented Jul 1, 2021

It's likely that the original error was caused by using scheme sqlite: rather than sqlite3:, which, I guess, causes Caqti not to find a driver for the scheme. I just ran into that myself.

Maybe this can be mitigated with the sql_pool helper examining the scheme (which it only has to do once, the first time it lazily sets up the pool). If it matches some list of known typical errors, it can print a warning.

@aantron
Copy link
Owner

aantron commented Jul 1, 2021

Aonther thing I just noticed is that if you declare the SQL pool but never take a connection from it, the database does not get created. This is likely to happen early in development, when first writing a handler:

let () =
  Dream.run
  @@ Dream.logger
  @@ Dream.sql_pool "sqlite3:db.sqlite"
  @@ Dream.router [
    Dream.get "/" (fun _ -> Dream.html "abc");
  ]
  @@ Dream.not_found

Once you call Dream.sql inside the handler, the database is created when the handler is triggered.

This level of laziness is part of how Dream tries to integrate transparently into larger applications without affecting them, but maybe this needs to be pointed out somewhere in the docs.

@thangngoc89
Copy link
Contributor Author

Thank you for taking a look into this. Let’s me change the tittle of this issue

@thangngoc89 thangngoc89 changed the title Create sqlite3 database automatically when not exists Meaningful error when using incorrect database url protocol Jul 1, 2021
@aantron aantron added this to the alpha3 milestone Jul 1, 2021
@aantron
Copy link
Owner

aantron commented Jul 2, 2021

Just adding a bit more detail on what needs to be done for the scheme sanity check.

Somewhere before this call:

Caqti_lwt.connect_pool ?max_size:size ~post_connect (Uri.of_string uri) in

...we need to take out the Uri.of_string uri sub-call, and, using ocaml-uri (which provides module Uri), examine the scheme. If the scheme is sqlite, we should probably print a warning to the dream.sql log, similar to

dream/src/sql/sql.ml

Lines 43 to 46 in 809835d

let message =
Printf.sprintf "Dream.sql_pool: cannot create pool for '%s': %s"
uri (Caqti_error.show error) in
log.error (fun log -> log ~request "%s" message);

suggesting that maybe the user meant sqlite3.

We can add more sanity checks as necessary.

Another thing to check is trying a connection string with no scheme at all. IIRC this generates a pretty useful error message, but it's good to check.

@aantron
Copy link
Owner

aantron commented Jul 3, 2021

I also tested what happens when there is no URI scheme (user tries to "connect to a file"):

Dream.sql_pool "db.sqlite"
03.07.21 05:08:56.330       dream.sql ERROR REQ 1 Dream.sql_pool: cannot create pool for 'db.sqlite': Cannot load driver for <db.sqlite>: Missing URI scheme.

it's a bit verbose, but at least the Caqti error says "Missing URI scheme," so I think we can just leave that as is for now.

I decided not to document automatic lazy database creation for now for two reasons:

  • I prefer to do the database rework first, which will, among other things, make it easier to use a connection pool that was created during app initialization, rather than internally by Dream.sql_pool. That will also affect when the database tends to be created.
  • The question of when the database will be created will probably come up less often, now that we are clearer about this other error which can masquerade as the database not being created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants