Diesel: One test fails when I run `bin/test` locally

[DNNX]

Setup

Versions

• Rust: rustc 1.31.0 (abe02cefd 2018-12-04)
• Diesel: v1.4.2 and 0b6c59e36cef3faea0184404e43e3db4739d1193 (latest master at the time of writing)
• Database: mysql Ver 8.0.17 for osx10.13 on x86_64 (Homebrew) - client libraries, 8.0.17 - server.
• Operating System docker host is macOS High Sierra 10.13.6 (17G65), not sure what OS mysql container runs on.

Feature Flags

• diesel: mysql

Problem Description

When I run bin/test, all the tests pass except one.

What are you trying to accomplish?

I want to have all tests green on my machine in order to be able to play with diesel source code and possibly contribute.

What is the expected output?

All tests are green.

What is the actual output?

$ cd diesel && RUST_TEST_THREADS=1 cargo test --no-default-features --features mysql

...

---- src/query_dsl/mod.rs - query_dsl::QueryDsl::inner_join (line 398) stdout ----
thread 'src/query_dsl/mod.rs - query_dsl::QueryDsl::inner_join (line 398)' panicked at 'test executable failed:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Ok([("Sean", "Sean\'s post"), ("Sean", "Sean is a jerk")])`,
 right: `Err(DatabaseError(__Unknown, "You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near \'|| ?)\' at line 1"))`', src/query_dsl/mod.rs:30:1
note: Run with `RUST_BACKTRACE=1` for a backtrace.

', librustdoc/test.rs:367:17
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    src/query_dsl/mod.rs - query_dsl::QueryDsl::inner_join (line 398)

test result: FAILED. 116 passed; 1 failed; 16 ignored; 0 measured; 0 filtered out

I tried it on the latest master and on v1.4.2. Same error.

Are you seeing any additional errors?

Nope.

Steps to reproduce

I followed the contributing guidelines when setting up everything locally. Here is what I did, precisely:

• Clone the repo and cd into it
• cp .env.sample .env, also bump all ports by 1, i.e. 5432 -> 5433 and 3306 -> 3307.
• Made the same changes in docker-compose.yml, so that ports from docker don't conflict with my local database servers.
• brew install mysql
• bin/test

Checklist

• [x] I have already looked over the issue tracker for similar issues.
• [x] This issue can be reproduced on Rust's stable channel.

[weiznich]

It seems like this issue is specific to mysql 8.0.17. We are already aware of this but nobody had enough time to look into it yet. Maybe I will find some time in the next few days. In the mean time just use 8.0.16 which should work fine.

See #2108 for some details.

[DNNX]

Yep, it worked. The following patch fixes the problem:

diff --git a/docker-compose.yml b/docker-compose.yml
index 80bd7b1a..9592e55a 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -1,7 +1,7 @@
 version: '3'
 services:
   mysql:
-    image: mysql
+    image: mysql:8.0.16
     container_name: diesel.mysql
     volumes:
       - "mysql-data:/var/lib/mysql/:delegated"

I also had to delete old docker volume with docker volume rm diesel_mysql-data because otherwise mysql wouldn't start.

Thank you for the prompt response and feel free to close this issue if it's a duplicate.

[DNNX]

Looking at the error message and MySQL changelog, this seems relevant:

https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-17.html#mysqld-8-0-17-deprecation-removal

Use of || is deprecated unless the PIPES_AS_CONCAT SQL mode is enabled. In that case, || signifies the SQL-standard string concatenation operator).

Although we do set PIPES_AS_CONCAT there: https://github.com/diesel-rs/diesel/blob/0b6c59e36cef3faea0184404e43e3db4739d1193/diesel/src/mysql/connection/mod.rs#L134... So it should still work.

[JohnTitor]

Thanks for your report, I'll take a look shortly.

[DNNX]

I updated docker-compose to run the two versions of mysql side by side on different ports, and created a small isolated script to show the difference between them. Mysql client needs to be installed locally in order to run them.

# Note: the two commands are only different by port number.
$ echo "SET sql_mode=(SELECT CONCAT(@@sql_mode, ',PIPES_AS_CONCAT')); select 'ab' like 'a' || '%'" | mysql --port 3307 --host localhost --protocol=TCP --user root diesel_test
'ab' like 'a' || '%'
1

$ echo "SET sql_mode=(SELECT CONCAT(@@sql_mode, ',PIPES_AS_CONCAT')); select 'ab' like 'a' || '%'" | mysql --port 3308 --host localhost --protocol=TCP --user root diesel_test
ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '|| '%'' at line 1

My docker-compose:

version: '3'
services:
  mysql:
    image: mysql:8.0.17
    container_name: diesel.mysql
    volumes:
      - "mysql-data:/var/lib/mysql/:delegated"
      - "./docker/mysql/init:/docker-entrypoint-initdb.d"
    ports:
      - "3308:3306"
    environment:
      - MYSQL_ALLOW_EMPTY_PASSWORD=true

  mysql-old:
    image: mysql:8.0.16
    container_name: diesel.mysql.old
    volumes:
      - "mysql-data-old:/var/lib/mysql/:delegated"
      - "./docker/mysql/init:/docker-entrypoint-initdb.d"
    ports:
      - "3307:3306"
    environment:
      - MYSQL_ALLOW_EMPTY_PASSWORD=true

volumes:
  mysql-data:
    driver: local
  mysql-data-old:
    driver: local

[weiznich]

So I've digged a bit further into this:

Enabling PIPES_AS_CONCAT seems to work fine on both versions because SELECT 'abc' || 'def'; fine for both.

The actual query that fails is this one:

SELECT `users`.`name`, `posts`.`title` FROM (`users` INNER JOIN `posts` ON `posts`.`title` LIKE `users`.`name` || '%');

Notably the concat operator is used in the on clause of an join. I was also able to reproduce this using the concat operator as part of a normal where clause, so the new version seems to have changed the behaviour their. I would say this is a bug of mysql side and should be reported there. Anyone interested in doing that?

Interestingly this is fixable by just adding a set of brackets around the strings for the concat operator. So the following query works fine:

SELECT `users`.`name`, `posts`.`title` FROM (`users` INNER JOIN `posts` ON `posts`.`title` LIKE (`users`.`name` || '%'));

I will think about what's the best way to work around that in short term on our end.

[DNNX]

I'll report a bug. Will post a link there later.

[JohnTitor]

Oh, good point, this query also works fine.
SET sql_mode=(SELECT CONCAT(@@sql_mode, ',PIPES_AS_CONCAT')); select 'ab' like ('a' || '%');

[DNNX]

https://bugs.mysql.com/bug.php?id=96405

[DNNX]

FWIW, I think I worked around the bug locally with slightly smaller changes than 56a0adf17827937325b48ac3f81c21fd72717248. The following patch fixes the test for 8.0.17, and 8.0.16 also works as before. Don't know if it's the right implementation though, maybe I did something stupid. But it compiles!

diff --git a/diesel/src/expression_methods/text_expression_methods.rs b/diesel/src/expression_methods/text_expression_methods.rs
index 55395bb2..9e1e31e8 100644
--- a/diesel/src/expression_methods/text_expression_methods.rs
+++ b/diesel/src/expression_methods/text_expression_methods.rs
@@ -1,6 +1,7 @@
 use expression::operators::{Concat, Like, NotLike};
 use expression::{AsExpression, Expression};
 use sql_types::{Nullable, Text};
+use expression::grouped::Grouped;

 /// Methods present on text expressions
 pub trait TextExpressionMethods: Expression + Sized {
@@ -55,8 +56,8 @@ pub trait TextExpressionMethods: Expression + Sized {
     /// assert_eq!(Ok(expected_names), names);
     /// # }
     /// ```
-    fn concat<T: AsExpression<Self::SqlType>>(self, other: T) -> Concat<Self, T::Expression> {
-        Concat::new(self, other.as_expression())
+    fn concat<T: AsExpression<Self::SqlType>>(self, other: T) -> Grouped<Concat<Self, T::Expression>> {
+        Grouped(Concat::new(self, other.as_expression()))
     }

     /// Returns a SQL `LIKE` expression

[weiznich]

Yes that also would work with the downside of changing the type signature of queries containing a concat operator. I would rather like to avoid changing that signature in a incompatible way, if there is another way, but probably other people from @diesel-rs/contributors have different opinion here.

[JohnTitor]

I think 56a0adf17827937325b48ac3f81c21fd72717248 seems better that it just changes the way || parses without touching fn concat.

[DNNX]

Looking at it - maybe it's a good idea to use parentheses more generously in general.

I just found another bug (in diesel this time). Consider this:

diff --git a/diesel/src/query_dsl/mod.rs b/diesel/src/query_dsl/mod.rs
index 584cb29d..db2c0f8d 100644
--- a/diesel/src/query_dsl/mod.rs
+++ b/diesel/src/query_dsl/mod.rs
@@ -415,6 +415,7 @@ pub trait QueryDsl: Sized {
     ///
     /// let data = users
     ///     .inner_join(posts.on(title.like(name.concat("%"))))
+    ///     .filter(title.eq("ABRA").eq(title.eq("CADABRA")))
     ///     .select((name, title))
     ///     .load(&connection);
     /// let expected_data = vec![

You can run the test with cd diesel && RUST_TEST_THREADS=1 cargo test --no-default-features --features postgres query_dsl::QueryDsl::inner_join.

This change should not break the test because title = "ABRA" is false for all rows in test data and title = "CADABRA" is also false. false = false should evaluate to true, so the generated WHERE shouldn't filter out any rows and the test should pass (even on 8.0.16).

It does break the test, however, because the generated condition evaluates to false, so no records are returned as a result. The reason is missing brackets.

-- this is generated
WHERE title = title = title = title

-- this should be generated instead
WHERE (title = title) = (title = title)

mysql> select (2=2)=(2=2), 2=2 = 2=2;
+-------------+-----------+
| (2=2)=(2=2) | 2=2 = 2=2 |
+-------------+-----------+
|           1 |         0 |
+-------------+-----------+
1 row in set (0.00 sec)

More than that, in postgres the title = title = title = title part yields a syntax error.

This example is a bit contrived, but I'm pretty sure there are other, more realistic cases for which missing parentheses mean broken SQL.