diesel icon indicating copy to clipboard operation
diesel copied to clipboard

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

Open violetastcs opened this issue 2 years ago • 18 comments

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] 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

violetastcs avatar Dec 17 '21 17:12 violetastcs

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/

weiznich avatar Dec 17 '21 17:12 weiznich

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.

Mingun avatar Dec 17 '21 18:12 Mingun

@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.

weiznich avatar Dec 17 '21 18:12 weiznich

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.

Mingun avatar Dec 17 '21 18:12 Mingun

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.

violetastcs avatar Dec 17 '21 19:12 violetastcs

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

weiznich avatar Dec 17 '21 19:12 weiznich

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 avatar Dec 17 '21 22:12 violetastcs

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

violetastcs avatar Dec 17 '21 22:12 violetastcs

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?

weiznich avatar Dec 19 '21 13:12 weiznich

@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.

czotomo avatar Mar 27 '22 01:03 czotomo

@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.

weiznich avatar Mar 28 '22 15:03 weiznich

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

czotomo avatar Mar 29 '22 18:03 czotomo

@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.

weiznich avatar Mar 30 '22 15:03 weiznich

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.

oslac avatar Jun 17 '22 14:06 oslac

@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.

weiznich avatar Jun 17 '22 20:06 weiznich

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.

MasterStaz avatar Apr 03 '23 21:04 MasterStaz

@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.

weiznich avatar Apr 04 '23 08:04 weiznich

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 :)

MasterStaz avatar Apr 04 '23 15:04 MasterStaz