sqlite rowid support

[ghost]

Setup

NixOS 19.03

[nix-shell:~/work]$ rustup --version
rustup 1.16.0 ( )

[nix-shell:~/work]$ rustc --version
rustc 1.38.0-nightly (c43d03a19 2019-08-14)

[nix-shell:~/work]$ cargo --version
cargo 1.38.0-nightly (e853aa976 2019-08-09)

Versions

• Rust:
• Diesel: 1.4.2
• Database: sqlite
• Operating System

Feature Flags

• diesel: sqlite, chrono

Problem Description

diesel doesn't know that sqlite generates a PK for each table called rowid. I don't need to specify a PK.

What are you trying to accomplish?

create a table

What is the expected output?

migrations are ok

What is the actual output?

Diesel only supports tables with primary keys. Table poster has no primary key

Are you seeing any additional errors?

no

Steps to reproduce

migration:

CREATE TABLE IF NOT EXISTS poster (
    key TEXT UNIQUE NOT NULL
);

running diesel migration run causes the error.

Checklist

• I have already looked over the issue tracker for similar issues.
• This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)

[sgrif]

diesel doesn't know that sqlite generates a PK for each table called rowid. I don't need to specify a PK.

This is because SQLite doesn't expose this to us. Tables can be created without rowids, and we have no way of knowing whether that was the case or not. You should explicitly specify a primary key if you'd like to use Diesel to access that table, otherwise you can add it to the list of tables to ignore.

[longsleep]

It would be nice to reopen this.

To figure out which tables are rowid table one can use something like this:

SELECT name, NOT EXISTS(SELECT 1 FROM pragma_index_xinfo(x.name)) AS 'hasRowid'
    FROM sqlite_schema AS x
    WHERE type='table'
    ORDER BY name;

With this information, diesel could implicitly add a primary key to the schema if there is none available explicitly.

[weiznich]

@longsleep Can you explain how that would be different to what's implemented in #3680?

[longsleep]

@longsleep Can you explain how that would be different to what's implemented in #3680?

Oh great thanks for pointing to that MR. I did not check the "unreleased" changes.

I just tested the current "master" and diesel print-schema generates an invalid schema as it only picks up rowid half the way.

CREATE TABLE store_properties (
	proptag INTEGER UNIQUE NOT NULL,
	propval BLOB NOT NULL);

generates:

diesel::table! {
    store_properties (rowid) {
        proptag -> Integer,
        propval -> Binary,
    }
}

Which is missing the rowid column for some reason. I have not yet figured out why that is, as the diesel test in https://github.com/diesel-rs/diesel/blob/master/diesel_cli/tests/print_schema/print_schema_sqlite_without_explicit_primary_key/sqlite/expected.snap#L9 seems to generate it as expected.

I will investigate further and open a dedicated ticket for this should be conclusion be that some functionality is still missing for implicit rowid support.

[longsleep]

It turned out that the test for this rowid feature is not used by the code base and hence its failure goes unnoticed.

When adding it:

cargo test -p diesel_cli -F sqlite --no-default-features print_schema
   Compiling diesel_cli v2.1.0 (/home/longsleep/Dev/diesel/diesel_cli)
    Finished test [unoptimized + debuginfo] target(s) in 3.19s
     Running unittests src/main.rs (/home/longsleep/Dev/diesel/target/debug/deps/diesel-64a7dbff111b712a)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 12 filtered out; finished in 0.00s

     Running tests/tests.rs (/home/longsleep/Dev/diesel/target/debug/deps/tests-6916b6ae4e9e11cd)

running 23 tests
stored new snapshot /home/longsleep/Dev/diesel/diesel_cli/tests/print_schema/print_schema_sqlite_without_explicit_primary_key/sqlite/expected.snap.new
test print_schema::print_schema_sqlite_without_explicit_primary_key ... FAILED
test print_schema::run_infer_schema ... ok
test print_schema::print_schema_reserved_names ... ok
test print_schema::print_schema_custom_types ... ok
test print_schema::run_infer_schema_django_bool_case ... ok
test print_schema::print_schema_column_renaming ... ok
test print_schema::print_schema_quoted_table_name ... ok
test print_schema::run_infer_schema_column_order ... ok
test print_schema::print_schema_with_unmappable_names ... ok
test print_schema::print_schema_several_keys_with_compound_key ... ok
test print_schema::print_schema_generated_columns_with_generated_always ... ok
test print_schema::print_schema_sqlite_implicit_foreign_key_reference ... ok
test print_schema::print_schema_with_separate_unique_constraint_and_foreign_key ... ok
test print_schema::print_schema_patch_file ... ok
test print_schema::print_schema_generated_columns ... ok
test print_schema::run_infer_schema_compound_primary_key ... ok
test print_schema::schema_file_is_relative_to_project_root ... ok
test print_schema::run_infer_schema_exclude ... ok
test print_schema::run_infer_schema_without_docs ... ok
test print_schema::run_infer_schema_include ... ok
test print_schema::run_infer_schema_exclude_regex ... ok
test print_schema::run_infer_schema_include_regex ... ok
test print_schema::run_infer_schema_table_order ... ok

failures:

---- print_schema::print_schema_sqlite_without_explicit_primary_key stdout ----
STDOUT: Creating migrations directory at: /tmp/print_schema_sqlite_without_explicit_primary_keyZrZYxl/migrations
Creating database: /tmp/print_schema_sqlite_without_explicit_primary_keyZrZYxl/print_schema_sqlite_without_explicit_primary_key

STDERR:
STDOUT: // @generated automatically by Diesel CLI.

diesel::table! {
    no_explicit (rowid) {
        name -> Nullable<Text>,
    }
}

diesel::table! {
    with_explicit_rowid (oid) {
        name -> Nullable<Text>,
        rowid -> Nullable<Text>,
    }
}

diesel::table! {
    with_explicit_rowid_oid (_rowid_) {
        name -> Nullable<Text>,
        rowid -> Nullable<Text>,
        oid -> Nullable<Text>,
    }
}

diesel::allow_tables_to_appear_in_same_query!(
    no_explicit,
    with_explicit_rowid,
    with_explicit_rowid_oid,
);

STDERR:
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: diesel_cli/tests/print_schema/print_schema_sqlite_without_explicit_primary_key/sqlite/expected.snap
Snapshot: expected
Source: diesel_cli/tests/print_schema.rs:391
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Test: print_schema_sqlite_without_explicit_primary_key
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    0     0 │ // @generated automatically by Diesel CLI.␊
    1     1 │ ␊
    2     2 │ diesel::table! {␊
    3     3 │     no_explicit (rowid) {␊
    4       │-        rowid -> Integer,␊
    5       │-        name -> Text,␊
          4 │+        name -> Nullable<Text>,␊
    6     5 │     }␊
    7     6 │ }␊
    8     7 │ ␊
    9     8 │ diesel::table! {␊
   10     9 │     with_explicit_rowid (oid) {␊
   11       │-        oid -> Integer,␊
   12       │-        name -> Text,␊
   13       │-        rowid -> Text,␊
         10 │+        name -> Nullable<Text>,␊
         11 │+        rowid -> Nullable<Text>,␊
   14    12 │     }␊
   15    13 │ }␊
   16    14 │ ␊
   17    15 │ diesel::table! {␊
   18    16 │     with_explicit_rowid_oid (_rowid_) {␊
   19       │-        _rowid_ -> Integer,␊
   20       │-        name -> Text,␊
   21       │-        rowid -> Text,␊
   22       │-        oid -> Text,␊
         17 │+        name -> Nullable<Text>,␊
         18 │+        rowid -> Nullable<Text>,␊
         19 │+        oid -> Nullable<Text>,␊
   23    20 │     }␊
   24       │-}
         21 │+}␊
         22 │+␊
         23 │+diesel::allow_tables_to_appear_in_same_query!(␊
         24 │+    no_explicit,␊
         25 │+    with_explicit_rowid,␊
         26 │+    with_explicit_rowid_oid,␊
         27 │+);␊
────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'print_schema::print_schema_sqlite_without_explicit_primary_key' panicked at /home/longsleep/.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.34.0/src/runtime.rs:563:9:
snapshot assertion for 'expected' failed in line 391
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    print_schema::print_schema_sqlite_without_explicit_primary_key

test result: FAILED. 22 passed; 1 failed; 0 ignored; 0 measured; 107 filtered out; finished in 0.26s

error: test failed, to rerun pass `-p diesel_cli --test tests`

This is the same result I get when trying with my table from above and the conclusion is that the feature implemented in #3680 is incomplete or at least partly non-functional.

[weiznich]

Thanks for trying that out. That likely means that I will remove the relevant code in the next few days. If there is interest in having this feature I'm open to accepting another PR for this.