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

Syntax error for delete operation #998

Closed
Undin opened this Issue Jul 5, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@Undin

Undin commented Jul 5, 2017

Setup

Versions

  • Rust: 1.18
  • Diesel: 0.14
  • Database: postgress
  • Operating System: macos

Feature Flags

  • diesel: postgres
  • diesel_codegen: postgres

Problem Description

It seems that generated SQL for delete operation is not valid in some cases.

For query which described below in Steps to reproduce section print_sql! macros prints next:

DELETE FROM subs
WHERE subs.user_id, subs.show_id IN
      (SELECT
         subs.user_id,
         subs.show_id
       FROM
         (users
           INNER JOIN (subs
             INNER JOIN shows ON subs.show_id = shows.id) ON subs.user_id = users.id)
       WHERE users.id = ? AND shows.title = ?)

but posgtres expects parens in WHERE clause:

DELETE FROM subs
WHERE (subs.user_id, subs.show_id) IN
      (SELECT
         subs.user_id,
         subs.show_id
       FROM
         (users
           INNER JOIN (subs
             INNER JOIN shows ON subs.show_id = shows.id) ON subs.user_id = users.id)
       WHERE users.id = ? AND shows.title = ?)

What are you trying to accomplish?

I'm trying to delete object from table with nested select.

What is the expected output?

Success! in console and successful execution of delete operation.

What is the actual output?

Err(DatabaseError(__Unknown, "syntax error at or near \",\""))

Steps to reproduce

up.sql

-- Your SQL goes here
CREATE TABLE users (
  id INTEGER PRIMARY KEY
);

CREATE TABLE shows (
  id BIGSERIAL PRIMARY KEY,
  title TEXT NOT NULL
);

CREATE TABLE subs (
  show_id BIGINT NOT NULL REFERENCES shows ON DELETE CASCADE,
  user_id INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
  PRIMARY KEY (show_id, user_id)
);

INSERT INTO users (id) VALUES
  (1);

INSERT INTO shows (id, title) VALUES
  (1, 'show-title');

INSERT INTO subs (show_id, user_id) VALUES
  (1, 1);

models.rs

use schema::{users, shows, subs};

#[derive(Identifiable, Queryable, Insertable, Associations, Debug)]
#[table_name="users"]
pub struct User {
    pub id: i32
}

#[derive(Identifiable, Queryable, Insertable, Associations, Debug)]
#[table_name="shows"]
pub struct Show {
    pub id: i64,
    pub title: String
}

#[derive(Queryable, Insertable, Associations, Debug)]
#[belongs_to(Show)]
#[belongs_to(User)]
#[table_name="subs"]
pub struct Subscription {
    pub show_id: i64,
    pub user_id: i32,
}

enable_multi_table_joins!(users, shows);

schema.rs

infer_schema!("dotenv:DATABASE_URL");

lib.rs

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

pub mod models;
pub mod schema;

use diesel::prelude::*;
use diesel::pg::PgConnection;
use dotenv::dotenv;
use std::env;

pub fn establish_connection() -> PgConnection {
    dotenv().ok();

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

main.rs

extern crate diesel_delete_example;
extern crate diesel;

use diesel::prelude::*;
use diesel_delete_example::*;
use diesel_delete_example::schema::*;

fn main() {

    use diesel_delete_example::schema::subs::*;
    use diesel_delete_example::schema::shows::title;
    use diesel_delete_example::schema::users::id;

    let test_user_id = 1;
    let test_title = "show-title";

    let query = diesel::delete(subs::table.filter((user_id, show_id).eq_any(
        users::table.inner_join((subs::table).inner_join(shows::table))
            .select((user_id, show_id))
            .filter(id.eq(test_user_id).and(title.eq(test_title))))
    ));

    let connection = establish_connection();
    match query.execute(&connection) {
        Ok(_) => println!("Success!"),
        Err(e) => println!("{:?}", e)
    }
}

Checklist

  • I have already looked over the issue tracker for similar issues.
@sgrif

This comment has been minimized.

Member

sgrif commented Jul 5, 2017

We need to make this fail to compile. We don't support the PG tuple type (and even if we did, it wouldn't map to a Rust tuple).

We should probably figure out a way to have all of the expression methods omitted from tuples.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 5, 2017

Also since you're probably looking for a workaround for what you're trying to do:

use diesel::expression::grouped::Grouped;
let query = diesel::delete(subs::table.filter(Grouped((user_id, show_id)).eq_any(
        users::table.inner_join((subs::table).inner_join(shows::table))
            .select((user_id, show_id))
            .filter(id.eq(test_user_id).and(title.eq(test_title))))
    ));

Grouped is internal, and that code may stop working in the future, but that will at least give you what you need for now. The way that I'd probably want to write that is:

let target = subs::table
    .filter(subs::user_id.eq(test_user_id))
    .filter(exists(shows::table.filter(shows::title.eq(test_title).and(shows::id.eq(subs::show_id)))

but that will fail to compile at the moment (supporting subselects which reference the outer table is something we're looking to fix)

@Undin

This comment has been minimized.

Undin commented Jul 5, 2017

Workaround works perfectly:)
Thanks a lot!

sgrif added a commit that referenced this issue Jul 6, 2017

Don't implement expression methods on tuples
With the exception of `.nullable`, all of these methods would generate
invalid SQL if used. Some people mistakenly think that we map rust
tuples to a PG tuple. We don't actually support PG tuples, and if we did
we'd need to have it be a separate type to differentiate `SELECT id,
name` from `SELECT (id, name)`.

This is not a full solution to the problem. Ideally we would actually
properly represent in the type system that a tuple is a list, which is
only valid in a few contexts. Instead, this solution simply prevents
their construction. I don't think this mistake is common enough to
warrant more time being spent on it.

My hope is that if/when variadic generics happen, they are not built
around tuples, which would allow us to better separate this case from
normal values. Until that time, this should suffice.

I'm not 100% sold on the name `SingleValue`, as it seems odd that
`Array<T>` implements it. However, the only other name I could think of
was `NotTuple`, which will feel odd if we ever do support PG tuples.
Something like `NotColumnList` could work, but that name feels odd to
me. I think I want to stick with a name that says what the type is, not
what it isn't.

Fixes #998.

sgrif added a commit that referenced this issue Jul 6, 2017

Don't implement expression methods on tuples
With the exception of `.nullable`, all of these methods would generate
invalid SQL if used. Some people mistakenly think that we map rust
tuples to a PG tuple. We don't actually support PG tuples, and if we did
we'd need to have it be a separate type to differentiate `SELECT id,
name` from `SELECT (id, name)`.

This is not a full solution to the problem. Ideally we would actually
properly represent in the type system that a tuple is a list, which is
only valid in a few contexts. Instead, this solution simply prevents
their construction. I don't think this mistake is common enough to
warrant more time being spent on it.

My hope is that if/when variadic generics happen, they are not built
around tuples, which would allow us to better separate this case from
normal values. Until that time, this should suffice.

I'm not 100% sold on the name `SingleValue`, as it seems odd that
`Array<T>` implements it. However, the only other name I could think of
was `NotTuple`, which will feel odd if we ever do support PG tuples.
Something like `NotColumnList` could work, but that name feels odd to
me. I think I want to stick with a name that says what the type is, not
what it isn't.

Fixes #998.

@sgrif sgrif closed this in #1002 Jul 6, 2017

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 6, 2017

FYI the workaround I gave you will no longer work in the next release. I'd do this instead (and this is probably just the best way to write the query regardless):

let shows_with_title = shows::table
    .select(shows::id)
    .filter(shows::title.eq(test_title));
let target = subs::table
    .filter(subs::user_id.eq(test_user_id))
    .filter(subs::show_id.eq_any(shows_with_title));
let query = diesel::delete(target);

The SQL will be:

DELETE FROM subs
  WHERE subs.user_id = $1
    AND subs.show_id IN (
      SELECT shows.id FROM shows WHERE shows.title = $2
    )

which should be significantly more efficient than what you were trying to write originally

Fiedzia added a commit to Fiedzia/diesel that referenced this issue Jul 9, 2017

Don't implement expression methods on tuples
With the exception of `.nullable`, all of these methods would generate
invalid SQL if used. Some people mistakenly think that we map rust
tuples to a PG tuple. We don't actually support PG tuples, and if we did
we'd need to have it be a separate type to differentiate `SELECT id,
name` from `SELECT (id, name)`.

This is not a full solution to the problem. Ideally we would actually
properly represent in the type system that a tuple is a list, which is
only valid in a few contexts. Instead, this solution simply prevents
their construction. I don't think this mistake is common enough to
warrant more time being spent on it.

My hope is that if/when variadic generics happen, they are not built
around tuples, which would allow us to better separate this case from
normal values. Until that time, this should suffice.

I'm not 100% sold on the name `SingleValue`, as it seems odd that
`Array<T>` implements it. However, the only other name I could think of
was `NotTuple`, which will feel odd if we ever do support PG tuples.
Something like `NotColumnList` could work, but that name feels odd to
me. I think I want to stick with a name that says what the type is, not
what it isn't.

Fixes diesel-rs#998.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment