Primary key always generates `Nullable<_>` in schema.rs

violetastcs commented 2 years ago

Setup

Versions

Rust: rustc 1.59.0-nightly (404c8471a 2021-12-14)
Diesel: 1.4.1
Database: SQLite
Operating System Ubuntu 21.10

Feature Flags

diesel: (no default features) sqlite

Problem Description

A primary key, whether NOT NULL is implied or written out explicitly, always results in Nullable

What are you trying to accomplish?

Generate the schema for

CREATE TABLE cards (
    id INTEGER PRIMARY KEY NOT NULL,
    -- The MTGJSON UUID for the card; all info is stored in MTGJSON db
    uuid VARCHAR UNIQUE NOT NULL
)

What is the expected output?

table! {
    cards (id) {
        id -> Integer,
        uuid -> Text,
    }
}

What is the actual output?

table! {
    cards (id) {
        id -> Nullable<Integer>,
        uuid -> Text,
    }
}

Are you seeing any additional errors?

No

Steps to reproduce

Create a new migration and add

CREATE TABLE cards (
    id INTEGER PRIMARY KEY NOT NULL,
    -- The MTGJSON UUID for the card; all info is stored in MTGJSON db
    uuid VARCHAR UNIQUE NOT NULL
)

to up.sql, and then run diesel migration run

Checklist

[x] I have already looked over the issue tracker and the disussion forum for similar possible closed issues.
[x] This issue can be reproduced on Rust's stable channel. (Your issue will be closed if this is not the case)
[x] This issue can be reproduced without requiring a third party crate

weiznich commented 2 years ago

Duplicate of https://github.com/diesel-rs/diesel/issues/1984

Also Sqlite primary keys are allowed to be nullable:

Based on the SQL standard, PRIMARY KEY should always imply NOT NULL. However, SQLite allows NULL values in the PRIMARY KEY column except that a column is INTEGER PRIMARY KEY column or the table is a WITHOUT ROWID table or the column is defined as a NOT NULL column.

This is due to a bug in some early versions. If this bug is fixed to conform with the SQL standard, then it might break the legacy systems. Therefore, it has been decided to allow NULL values in the PRIMARY KEY column.

https://www.sqlitetutorial.net/sqlite-not-null-constraint/

Mingun commented 2 years ago

While this is documented sqlite behavior, probably it will be best to add a feature flag that enables generation of more predictable result if so many users complain about that strange "feature" which hardly ever used in real world and probably never used in any diesel applications.

Also, the quoted text says that in that case column should not to be nullable:

However, SQLite allows NULL values in the PRIMARY KEY column except that a column is INTEGER PRIMARY KEY column or the table is a WITHOUT ROWID table or the column is defined as a NOT NULL column.

weiznich commented 2 years ago

@Mingun To be clear here: We just generate types bases on what sqlite is telling us through PRAGMA table_info('...'). If you believe that this is the wrong output please raise an issue at the sqlite issue tracker.

While this is documented sqlite behavior, probably it will be best to add a feature flag that enables generation of more predictable result if so many users complain about that strange "feature" which hardly ever used in real world and probably never used in any diesel applications

Again: Please write a complete proposal in the discussion section of the repo instead of commenting on a random issue. If you do not want to spend the time to write such an proposal there is really no need to comment here in the first place. I will not accept this behavior much longer, so consider that as a warning.

Mingun commented 2 years ago

You feel free to convert this issue to a discussion if you really want listen for proposals.

Also, it seems you completely ignore the second part of my comment. I repeat it here:

Also, the quoted text says that in that case column should not to be nullable:

However, SQLite allows NULL values in the PRIMARY KEY column except that a column is INTEGER PRIMARY KEY column or the table is a WITHOUT ROWID table or the column is defined as a NOT NULL column.

violetastcs commented 2 years ago

Duplicate of #1984

Also Sqlite primary keys are allowed to be nullable:

Based on the SQL standard, PRIMARY KEY should always imply NOT NULL. However, SQLite allows NULL values in the PRIMARY KEY column except that a column is INTEGER PRIMARY KEY column or the table is a WITHOUT ROWID table or the column is defined as a NOT NULL column. This is due to a bug in some early versions. If this bug is fixed to conform with the SQL standard, then it might break the legacy systems. Therefore, it has been decided to allow NULL values in the PRIMARY KEY column.

https://www.sqlitetutorial.net/sqlite-not-null-constraint/

I don't think this is a duplicate, they don't use NOT NULL where I do. In this case regardless, though, with an explicit NOT NULL, the field should not be Nullable, at least unless I've missed something else.

Also, while playing around with this to try and see if I could find the issue, I've encountered more strange behavior; given the input

CREATE TABLE collection_cards (
    id INTEGER PRIMARY KEY NOT NULL,
    collection INTEGER NOT NULL,
    uuid VARCHAR NOT NULL
)

I get the schema

table! {
    collection_cards (id) {
        id -> Nullable<Integer>,
        collection -> Nullable<Integer>,
        uuid -> Nullable<Text>,
    }
}

despite all of the fields being marked as NOT NULL. I'll update the issue when I have time to correct myself and add this extra information.

weiznich commented 2 years ago

@violetastcs Oh, thats something I've missed. Can you provide the corresponding output of PRAGMA table_info('collection_cards')?

violetastcs commented 2 years ago

when I run PRAGMA table_info('collection_cards'), I get

0|id|INTEGER|99||1
1|collection|INTEGER|99||0
2|uuid|VARCHAR|99||0

violetastcs commented 2 years ago

Just noticed this too: I have the SQL

CREATE TABLE deck_cards (
    id INTEGER PRIMARY KEY NOT NULL,
    deck INTEGER NOT NULL,
    uuid VARCHAR NOT NULL
)

which generates

table! {
    deck_cards (id) {
        id -> Integer,
        deck -> Integer,
        uuid -> Text,
    }
}

which seems to work while the others don't

weiznich commented 2 years ago

That looks strange. The fourth column returned by PRAGMA table_info('…') is documented as boolean indicator if the column is not null or not. So it should contain a zero or a one. Your result somehow contains a 99. I don't see this documented anywhere in the sqlite documentation. I've also tried to reproduce this locally using the provided migration and sqlite 3.35.5 but failed to get even this result. Could you add additional information about your environment + your sqlite version?

czotomo commented 2 years ago

@weiznich For what it's worth, I was able to reproduce the value 99 using SQLite version 2.8.17 (sqlite CLI tool) but it works correctly when I execute create table statement under SQLite 3.31.1 (sqlite3 CLI tool). My system: Ubuntu 20.04 over WSL. SQLite was installed using apt repos for this Ubuntu.

weiznich commented 2 years ago

@czotomo Thanks for looking into this.

That sounds like an sqlite version issue then. I believe diesel requires at least sqlite version 3.20, so one could argue that sqlite 2.8.17 is not supported. After all that's an sqlite version from 2005. Database created with that sqlite version cannot even be opened with the current version of the sqlite3 tool. I'm not even sure how the OP managed to get diesel into reading these files at all. Therefore I propose that we close this issue as wont-fix due to the ancient version of sqlite.

czotomo commented 2 years ago

Unless OP can provide additional information, one of which would be that they did not use SQLite version 2, I suggest closing, too.

weiznich commented 2 years ago

@violetastcs Maybe could you provide information about the used sqlite version? Otherwise I will close this next week as wont-fix as it seems to use an ancient sqlite version.

oslac commented 2 years ago

This behavior seems strange to me as the sqlite example somehow has generated schema as non-nullable when using INTEGER PRIMARY KEY AUTOINCREMENT

Using the up.sql from the example, and running migration generates id -> Nullable<Integer> for me.

Running PRAGMA table_info('users'); displays:

0|id|INTEGER|0||1
1|name|TEXT|1||0
2|hair_color|TEXT|0||0
3|created_at|TIMESTAMP|1|CURRENT_TIMESTAMP|0
4|updated_at|TIMESTAMP|1|CURRENT_TIMESTAMP|0

What I expected: for the generated schema to show id -> Integer instead of id -> Nullable<Integer> as in the official examples. Currently it would force me to use Option<i32> on the id of a queryable User struct, which is bit comical innit.

Diesel CLI version: diesel 1.4.1 diesel lib version 1.4.8 Sqlite3 version: sqlite 3.31.1 rustc version: rustc 1.63.0-nightly WSL2.

weiznich commented 2 years ago

@oslac This issue is not about the case you are talking about, as it is about schema inference issues for primary keys with an explicit not null constraint. Please stop posting things to unrelated issues. Please use the support forum if you want to ask questions.

MasterStaz commented 1 year ago

Just ran into the same behavior. Versions

Rust: rustc 1.68.2 (9eb3afe9e 2023-03-27)
Diesel CLI: 2.0.1
Database: SQLite 3.31.1
Operating System Zorin OS 16.2 (Codename: focal)

2023-04-03-195512_create_user/up.sql

CREATE TABLE IF NOT EXISTS user(
    id INTEGER NOT NULL PRIMARY KEY,
    name TEXT NOT NULL,
    fist_name TEXT,
    birthdate TEXT,
    rank TEXT
);

2023-04-03-202439_create_user_credentials/up.sql

CREATE TABLE IF NOT EXISTS user_credentials (
    id INTEGER NOT NULL PRIMARY KEY,
    username TEXT NOT NULL UNIQUE,
    password TEXT NOT NULL
);

schema.rs

// @generated automatically by Diesel CLI.

diesel::table! {
    user (id) {
        id -> Nullable<Binary>,
        name -> Nullable<Text>,
        fist_name -> Nullable<Text>,
        birthdate -> Nullable<Text>,
        rank -> Nullable<Text>,
    }
}

diesel::table! {
    user_credentials (id) {
        id -> Integer,
        username -> Text,
        password -> Text,
    }
}

diesel::allow_tables_to_appear_in_same_query!(
    user,
    user_credentials,
);

I already tried regenerating it, including deleting the schema.rs and running the generation again and copying the id line. As you see even the data type isn't correct. I hope this helps a bit.

weiznich commented 1 year ago

@MasterStaz Can you provide a database file that produces the issue locally for you. Also can you try to build a diesel-cli variant with the sqlite-bundled feature flag enabled? That should show whether that's a issue with this specific sqlite version or something different.

MasterStaz commented 1 year ago

It seems that it is fixed now. What I did: I wanted a clean slate, so I deleted the sqlite db-File and the schema.rs. After that I generated both again using diesel migration run. I got the following schema.rs:

// @generated automatically by Diesel CLI.

diesel::table! {
    user (id) {
        id -> Integer,
        name -> Text,
        fist_name -> Nullable<Text>,
        birthdate -> Nullable<Text>,
        rank -> Nullable<Text>,
    }
}

diesel::table! {
    user_credentials (id) {
        id -> Integer,
        username -> Text,
        password -> Text,
    }
}

diesel::allow_tables_to_appear_in_same_query!(
    user,
    user_credentials,
);

Sorry for bothering you :)

diesel-rs / diesel