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

Binding a null value should never fail #3558

Closed
3 tasks done
aeblyve opened this issue Mar 23, 2023 · 2 comments
Closed
3 tasks done

Binding a null value should never fail #3558

aeblyve opened this issue Mar 23, 2023 · 2 comments

Comments

@aeblyve
Copy link

aeblyve commented Mar 23, 2023

Setup

Versions

  • Rust:
    rustc 1.68.0
  • Diesel:
    2.0.0
  • Database:
    Sqlite
  • Operating System
    NixOS

Feature Flags

  • diesel:
    ["sqlite", "r2d2"]

Problem Description

What are you trying to accomplish?

Perform a recursive CTE query to load an item from a path spec, using sql_query and a struct deriving QueryableByName

What is the expected output?

The columns desired are selected

What is the actual output?

thread 'rocket-worker-thread' panicked at 'Binding a null value should never fail. If you ever see this error message please open an issue at diesels issue tracker containing code how to trigger this message.: DatabaseError(Unknown, "column index out of range")', /home/leonid/.cargo/registry/src/index.crates.io-6f17d22bba15001f/diesel-2.0.3/src/sqlite/connection/stmt.rs:344:22

Are you seeing any additional errors?

N/A

Steps to reproduce

main.rs

use diesel::sql_types::Text;
extern crate diesel;
use diesel::sqlite::SqliteConnection;
use diesel::{prelude::*, sql_query};
use dotenvy::dotenv;
use std::env;

mod schema;
mod models;

pub fn establish_connection() -> SqliteConnection {
    dotenv().ok();
    let database_url = env::var("DATABASE_URL").expect("DATABASE_URL must be set");
    SqliteConnection::establish(&database_url)
        .unwrap_or_else(|_| panic!("Error connecting to {}", database_url))
}

pub fn main() {
    let connection = &mut establish_connection();
    use self::models::Page;

    use self::schema::pages::dsl::*;

    // RECURSIVE SQL QUERY

    // Get all paths: then, compare with the path provided
    let child = sql_query(
        r#"WITH RECURSIVE CTE AS (
             SELECT id, slug AS path
             FROM pages
             WHERE parent_id IS NULL
             UNION ALL
             SELECT p.id, path || '/' || slug
             FROM pages p
             JOIN CTE ON p.parent_id = CTE.id
           )
           SELECT *
           FROM CTE
           WHERE path = '?'
"#
    );
    let path = String::from("foo");
    println!("path spec is {:?}", path);
    let child = child.bind::<Text, _>(path).load::<Page>(connection);
    println!("Child is: {:?}", child);
}

models.rs

use crate::schema::pages;
use diesel::{prelude::*};
use diesel::sql_types::{Nullable, Integer, Text};

#[derive(Queryable, QueryableByName, Insertable, Debug)]
#[diesel(primary_key(id))]
#[diesel(table_name = pages)]
pub struct Page {
    #[diesel(sql_type = Nullable<Integer>)]
    pub id: Option<i32>,
    #[diesel(sql_type = Nullable<Integer>)]
    pub parent_id: Option<i32>,
    #[diesel(sql_type = Text)]
    pub title: String,
    #[diesel(sql_type = Text)]
    pub slug: String,
    #[diesel(sql_type = Text)]
    pub html_content: String,
}

schema.rs

// @generated automatically by Diesel CLI.

diesel::table! {
    pages (id) {
        id -> Nullable<Integer>,
        parent_id -> Nullable<Integer>,
        title -> Text,
        slug -> Text,
        html_content -> Text,
    }
}

up.sql for pages

CREATE TABLE pages (
  id INTEGER PRIMARY KEY AUTOINCREMENT,
  parent_id INTEGER,
  title TEXT NOT NULL,
  slug TEXT NOT NULL,
  html_content TEXT NOT NULL,
  FOREIGN KEY(parent_id) REFERENCES pages(id) ON DELETE CASCADE
)

Database contains one page row in table pages,

1 | NULL | foo | foo | <p>bar</p>

Checklist

  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
  • This issue can be reproduced without requiring a third party crate

I use dotenvy to set up the db, but it is not required.

@aeblyve aeblyve added the bug label Mar 23, 2023
@weiznich
Copy link
Member

Thanks for opening this issue.

It seems like you triggered an internal error that is not supposed to be triggered. That written: It does not change the result, your query will error anyway because it does not contain a bind parameter.

Fixed query: (note the removed ' around the ?, that turns the string '?' into a bind parameter).

    let child = sql_query(
        r#"WITH RECURSIVE CTE AS (
             SELECT id, slug AS path
             FROM pages
             WHERE parent_id IS NULL
             UNION ALL
             SELECT p.id, path || '/' || slug
             FROM pages p
             JOIN CTE ON p.parent_id = CTE.id
           )
           SELECT *
           FROM CTE
           WHERE path = ?
"#,
    );

I'm leaving this issue open for now, because the error message should be improved in diesel.

@weiznich
Copy link
Member

I've opened #3560 to fix the panic.

weiznich added a commit that referenced this issue Mar 31, 2023
adamchalmers pushed a commit to adamchalmers/diesel that referenced this issue Apr 14, 2023
This commit fixes an issue where out of bound text/binary binds on `sql_query` using
the sqlite backend causes a panic in an internal drop impl. It was
caused that we pushed to the list of released binds before we actually
checked whether that bind was possible or not. This caused us trying to
bind to the same invalid bind again in the drop impl, while being on
the normal error path. It's fixed by just moving the relevant push after
the actual bind. In addition a regression test for this problem is added.
adamchalmers pushed a commit to adamchalmers/diesel that referenced this issue Apr 14, 2023
This commit fixes an issue where out of bound text/binary binds on `sql_query` using
the sqlite backend causes a panic in an internal drop impl. It was
caused that we pushed to the list of released binds before we actually
checked whether that bind was possible or not. This caused us trying to
bind to the same invalid bind again in the drop impl, while being on
the normal error path. It's fixed by just moving the relevant push after
the actual bind. In addition a regression test for this problem is added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants