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

Empty changeset is still executed, fails. #598

Closed
Boscop opened this Issue Jan 31, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@Boscop

Boscop commented Jan 31, 2017

When an update is done with an empty changeset (all options None), it still tries to execute the query (which fails, at least with postgres), maybe that should be prevented earlier?

@sgrif

This comment has been minimized.

Member

sgrif commented Feb 1, 2017

This should have been fixed in 0.9. Can you provide a script to reproduce?

@Eijebong

This comment has been minimized.

Member

Eijebong commented Aug 5, 2017

I just tried it and got

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: QueryBuilderError(StringError("There are no changes to save. This query cannot be built"))', /checkout/src/libcore/result.rs:860:4

I think this can be closed as we can't detect that there are no changes earlier than that.

@Boscop Feel free to reopen if you think I'm wrong

@Eijebong Eijebong closed this Aug 5, 2017

@sgrif

This comment has been minimized.

Member

sgrif commented Aug 5, 2017

Just to clarify on why it is important that this is an error case, while inserting an empty vector is not:

When you insert 0 rows, we always know exactly how to replicate the behavior that would have otherwise occurred (execute returns Ok(0), get_results returns Ok(Vec::new()), and get_result returns Err(NotFound)). That is not the case with a changeset with no changes.

In the absolute simplest case we'd need to do the equivalent SELECT statement to figure out what to return, but even that is insufficient. We'd also need to vary our behavior based on whether the changeset given was attempting to use upsert. Even with all of that, the triggers executed could potentially change the outcome as well.

@csharad

This comment has been minimized.

Contributor

csharad commented Sep 16, 2017

#[macro_use] extern crate diesel;
#[macro_use] extern crate diesel_codegen;
extern crate dotenv;

use diesel::prelude::*;
use diesel::mysql::MysqlConnection;
use dotenv::dotenv;
use std::env;
use schema::numbers;
pub fn establish_connection() -> MysqlConnection {
    dotenv().ok();

    let database_url = env::var("DATABASE_URL")
        .expect("DATABASE_URL must be set");
    MysqlConnection::establish(&database_url)
        .expect(&format!("Error connecting to {}", database_url))
}

mod schema {
    infer_schema!("dotenv:DATABASE_URL");
}

#[derive(Debug, Queryable)]
struct Number {
    id: i32,
    number: i32
}

#[derive(Insertable)]
#[table_name="numbers"]
pub struct NewNumber {
    number: i32
}

#[derive(AsChangeset)]
#[table_name="numbers"]
pub struct ChangeNumber {
    number: Option<i32>
}

fn main() {
    let connection = establish_connection();
    let results = numbers::table.limit(5).load::<Number>(&connection).expect("error");
    println!("{:?}", results);

    let n = NewNumber { number: 1 };
    diesel::insert(&n).into(numbers::table).execute(&connection).expect("errr");

    let change = ChangeNumber { number: None };
    diesel::update(numbers::table.find(1)).set(&change).execute(&connection).unwrap(); 
}

This code ends up with the same error but for mysql.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment