Skip to content

Add support for sqlite strict mode. #97

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

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Conversation

fire
Copy link
Contributor

@fire fire commented Jan 8, 2023

For review. I am not sure how to enable strict mode. I added the lines options: "STRICT" to force strict mode.

This fixes problems with uuids, booleans and datetimes on strict mode. We must use the correct sql types.

Help on how to run the test suite.

defmodule Uro.Repo.Migrations.CreateUsers do
  use Ecto.Migration

  def change do
    create table(:users, primary_key: false, options: "STRICT, WITHOUT ROWID") do
      add :id, :uuid, primary_key: true
      add :email, :string, null: false
      add :password_hash, :string

      add :username, :string
      add :display_name, :string

      add :email_notifications, :boolean, default: false

      add :profile_picture, :string
      add :is_admin, :boolean

      timestamps()
    end

    create unique_index(:users, :username)
    create unique_index(:users, :email)
  end
end

https://www.sqlite.org/draft/stricttables.html

@fire
Copy link
Contributor Author

fire commented Jan 8, 2023

The tests pass but looking for ideas on how to test strict mode and how to enable strict mode as a config option.

@fire fire mentioned this pull request Jan 8, 2023
4 tasks
@fire
Copy link
Contributor Author

fire commented Jan 8, 2023

@warmwaffles nudge

@warmwaffles
Copy link
Member

@fire I had a bit of a family emergency come up over the weekend. I'll be able to take a look at this tomorrow.

Copy link
Member

@warmwaffles warmwaffles left a comment

Choose a reason for hiding this comment

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

I like these changes. My BIGGEST concern is backwards compatibility with existing sqlite databases that were using this prior to the change.

@warmwaffles
Copy link
Member

@fire according to the sqlite documentation, the table option is all you need. There is no way to enable it by default upon opening an sqlite database.

Generally speaking I love strict types, but I do appreciate that sqlite allows us to specify custom types in the database in the way we do.

But I say let's merge this.

* Handle TEXT_UUID to TEXT.
* A uuid is TEXT or BLOB.
* Swap :datetime since it is passed to the database directly.
* The type ecto uses is :utc_datetime.
* A boolean is an INTEGER.
@fire
Copy link
Contributor Author

fire commented Jan 10, 2023

Can you help make strict table test?

@fire
Copy link
Contributor Author

fire commented Jan 10, 2023

Ready to go!

@warmwaffles
Copy link
Member

Yea @fire I'll throw together some tests and commit them to main after this is merged.

@warmwaffles warmwaffles merged commit 8f7de49 into elixir-sqlite:main Jan 10, 2023
@fire fire deleted the strict branch January 10, 2023 03:48
@warmwaffles
Copy link
Member

@fire when I get some time today, I'll sling together some more integration tests for strict vs non strict. Once that's done, I'll cut major release since this does break backwards compatibility some what.

@warmwaffles
Copy link
Member

@fire I added a note to the changelog about the breaking change under the unreleased section. I think I am going to cut a v1.0.0-rc1 and that can be tested out for a bit.

@warmwaffles
Copy link
Member

@fire so I have been chatting with @jeregrine about strict mode. Is there a specific hard requirement as why it is necessary or just a nice to have?

One of the reasons I originally left out the strict mode was being able to handle TEXT_DATETIME differently than just TEXT but also make it apparent to anyone who dumped the schema that the field is intended to be a DATETIME.

Another thing we sort of lose is the ability to specify a field as an ARRAY aka store it as json but deserialize with json_array() and the ability to specify that a field is a JSON field.

@fire
Copy link
Contributor Author

fire commented Jan 25, 2023

I wanted to be able to enforce strict and I didn't think starting a fork was the right answer.

Is there something we can do about the loss?

@fire
Copy link
Contributor Author

fire commented Jan 25, 2023

The request for sql strict mode is because I was using postgresql and most other sql engines don't offer the type flexibility of sqlite. So the reasoning are more religious in nature to match what the consensus sql developers have.

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