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

[sqlite] Syntax error in generated subexpression #1308

Closed
mfr-itr opened this Issue Nov 21, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@mfr-itr

mfr-itr commented Nov 21, 2017

Setup

Versions

  • Rust: 1.21
  • Diesel: 0.16
  • Database: sqlite
  • Operating System: linux

Problem Description

My real code has a structure similar to

    let now = diesel::select(diesel::expression::now);
    let query = diesel::select(diesel::expression::now).filter(diesel::expression::now.eq(now));
    println!("{:?}", diesel::debug_query::<diesel::sqlite::Sqlite,_>(&query).to_string());

which produces

SELECT CURRENT_TIMESTAMP WHERE CURRENT_TIMESTAMP = SELECT CURRENT_TIMESTAMP;

(syntax error) instead of

SELECT CURRENT_TIMESTAMP WHERE CURRENT_TIMESTAMP = (SELECT CURRENT_TIMESTAMP);

Checklist

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

How can I make it produce valid code while waiting for the fix? The sub-expression is a primary key lookup.

@bgeron

This comment has been minimized.

bgeron commented Feb 9, 2018

It seems that at least one maintainer says that it's a mistake that your program compiles, until someone has the time to put a lot of work in making this work properly. (Although I think your program is fine.)

I can see two workarounds to allow sub-selects in queries. I'm personally using the second.

Workaround 1

Write your SQL by hand. I believe you can still use Diesel to convert the results into Rust structs.

Workaround 2

Wrap your now in parentheses manually. For instance, you can use the following code

use diesel::{Expression, AppearsOnTable, QueryResult};
use diesel::backend::Backend;
use diesel::query_builder::{AstPass, QueryFragment};

pub fn parens<T: Expression>(e: T) -> Parens<T> {
    Parens(e)
}

#[derive(Debug, Copy, Clone)]
pub struct Parens<T: Expression>(T);

impl<T: Expression> Expression for Parens<T> {
    type SqlType = T::SqlType;
}

impl<QS, T: AppearsOnTable<QS>> AppearsOnTable<QS> for Parens<T> {}

impl<DB: Backend, T: Expression + QueryFragment<DB>> QueryFragment<DB> for Parens<T> {
    fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
        out.push_sql("(");
        self.0.walk_ast(out.reborrow())?;
        out.push_sql(")");
        Ok(())
    }
}    

after which you can simply write my_query.filter(my_field.eq(parens(now))).

@bgeron

This comment has been minimized.

bgeron commented Feb 9, 2018

If the maintainers agree that this bug should not be fixed, then they should probably close it as WONTFIX or something.

@sgrif

This comment has been minimized.

Member

sgrif commented Feb 10, 2018

@bgeron You are coming dangerously close to trolling. Last warning.

@bgeron

This comment has been minimized.

bgeron commented Feb 11, 2018

Sean, I'm a user who had the same problem, I managed to solve it for myself, and so I thought I'd share it with @mfr-itr. And I thought this 2 month old ticket could use an update.

I have to admit I didn't fully understand what you said on Gitter, even after reading it multiple times, so perhaps my summary is wrong. How about you write one and I remove mine?

sgrif added a commit that referenced this issue Feb 14, 2018

Allow (some) subselects to appear in an expression context
A subselect can be used as an expression by wrapping it in parenthesis
as long as the query returns exactly one column, and exactly zero or one
rows. The first constraint is trivial for us to enforce, but the only
way we can enforce the second is by automatically adding `LIMIT 1` to
the query when this method is called. We also need to make the SQL type
nullable, for the same reasons as max, min, avg, etc. They all return
null if the query returns zero rows.

If disjointness on associated types ever lands, I suspect we can just
implement `AsExpression` directly, and deprecate this method. That
said, given that the SQL type has to become nullable, it may be better
to keep the explicit method, so that we don't get questions on why
`id.eq(subselect)` isn't working.

Fixes #1308.

@sgrif sgrif closed this in #1555 Feb 17, 2018

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