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

New Sqlite3 driver #3

Merged
merged 10 commits into from
Oct 11, 2022
Merged

New Sqlite3 driver #3

merged 10 commits into from
Oct 11, 2022

Conversation

maiste
Copy link
Collaborator

@maiste maiste commented Oct 10, 2022

This PR introduces a driver for Sqlite3. It relies on sqlite3-ocaml to stay as low level as possible.

It requires #2 to be merged to support this new driver.

Copy link
Collaborator Author

@maiste maiste left a comment

Choose a reason for hiding this comment

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

Thanks for the formatting, I always forget to run dune fmt to format dune files as the code is autoformatted on my editor 😅

@tmattio
Copy link
Owner

tmattio commented Oct 11, 2022

Thanks @maiste!

I tried to run the driver on example migrations (that I pushed to the example/ directory) and I have the following error:

dune exec omigrate -- setup --source ./example --database sqlite3://./db.sqlite3
omigrate: internal error, uncaught exception:
          Sqlite3.Error("error opening database: unable to open database file")

Do you know what's going wrong?

@maiste
Copy link
Collaborator Author

maiste commented Oct 11, 2022

I replicate the issue: it seems to struggle with the relative path. If you put an absolute path, it will work. Investigate the way to fix that.

Update: Uri.path remove the . from ./<path>

Update-2: This is not supported as it's part of the RFC not to support it:

The complete path segments "." and ".." are intended only for use
within relative references (Section 4.1) and are removed as part of
the reference resolution process (Section 5.2) 

[...]

 1.  The input buffer is initialized with the now-appended path
       components and the output buffer is initialized to the empty
       string.

   2.  While the input buffer is not empty, loop as follows:

       A.  If the input buffer begins with a prefix of "../" or "./",
           then remove that prefix from the input buffer; otherwise,

       B.  if the input buffer begins with a prefix of "/./" or "/.",
           where "." is a complete path segment, then replace that
           prefix with "/" in the input buffer; otherwise,

       C.  if the input buffer begins with a prefix of "/../" or "/..",
           where ".." is a complete path segment, then replace that
           prefix with "/" in the input buffer and remove the last
           segment and its preceding "/" (if any) from the output
           buffer; otherwise,

       D.  if the input buffer consists only of "." or "..", then remove
           that from the input buffer; otherwise,

       E.  move the first path segment in the input buffer to the end of
           the output buffer, including the initial "/" character (if
           any) and any subsequent characters up to, but not including,
           the next "/" character or the end of the input buffer.

   3.  Finally, the output buffer is returned as the result of
       remove_dot_segments.

IMHO, we have to keep it this way but provide an example about the right way to write a URI for sqlite3, which is:

$ omigrate setup -d "sqlite3://$PWD/my_db.db" -s "./example"

WDYT @tmattio?

@tmattio
Copy link
Owner

tmattio commented Oct 11, 2022

Thanks @maiste! Sorry for the noise, I thought it was supported, but if the RFC is explicit about not supporting it, we shouldn't either!

I still have an error running the migrations though. After cleaning the SQL bits from the migrations that are not supported by sqlite3, I have the following error:

dune exec omigrate -- setup -d "sqlite3://$PWD/my_db.db" -s "./example"
omigrate: internal error, uncaught exception:
          Sqlite3.Error("Sqlite3.prepare: near \"(\": syntax error")

@maiste
Copy link
Collaborator Author

maiste commented Oct 11, 2022

I pushed some changes to fix this bug. The README is not supposed to be in the migration repository, which triggers the bug. To clarify the structure, I've split example into two subdirectories, postgres and sqlite3. This way, you can run the example with the following :

dune exec omigrate -- setup -d "sqlite3://$PWD/my_db.db" -s "./example/sqlite3"

As postgres and sqlite3 don't support the same features, the sqlite3 are the same as in go-migrate.

Warning
Even if it works, we should wait before merging; the history needs to be rewritten to make it clearer.

@maiste maiste mentioned this pull request Oct 11, 2022
Copy link
Owner

@tmattio tmattio left a comment

Choose a reason for hiding this comment

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

Looks great!

@tmattio tmattio merged commit 5fd1884 into tmattio:master Oct 11, 2022
@maiste maiste deleted the driver/sqlite3 branch October 11, 2022 15:33
tmattio added a commit to tmattio/opam-repository that referenced this pull request Oct 11, 2022
CHANGES:

- Support partial migrations (tmattio/omigrate#4, @maiste)
- New Sqlite3 driver (tmattio/omigrate#3, @maiste)
- Support optional auth with psql driver (tmattio/omigrate#2, @maiste)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants