Use database type conversion in Filters (test postgres+mysql) #1480

soyuka · 2017-11-06T09:40:33Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	na
License	MIT
Doc PR	na

Stated in Advanced field value conversion using custom mapping types:

When using DQL queries, the convertToPHPValueSQL and convertToDatabaseValueSQL methods only apply to identification variables and path expressions in SELECT clauses. Expressions in WHERE clauses are not wrapped!

We won't see any difference in sqlite for example (know why here) but I'm definitely seeing one in Oracle where the date parameter has the wrong format and therefore leads to a bad query. Maybe we should use this also in numeric filters.

/edit:

Add something to skip the ordering of relations while comparing values in JSON
Adding postgresql to travis:
I wouldn't think this would be so hard haha. Few bugs discovered:
- filtering on any integer field with a string containing a decimal (eg9.3) will throw (fixed but changes behavior as they're ignored)
- actually sqlite LIKE filters are case insensitive whereas postgres is case sensitive. This renders a few tests useless on sqlite.
- in features/numeric_filters we DO NOT test the numeric filter but only the search filter... (renamed)
- add numeric filters tests
- there is weird stuff going on in search_filter which should not be there (mainly on id and associations) maybe this should've been called text_filter. For example we can't say that an identifier is always named id assumption made here (fixed by using metadata)
- needs more tests on AbstractFilter (convert) + test iriconvert new method
Why does tags.feature fail on postgresql?

And the header "Cache-Tags" should not exist
And "/relation_embedders,/related_dummies,/third_levels" IRIs should be purged
IRIs "/relation_embedders,/related_dummies/1,/related_dummies,/third_levels/1,/third_levels" does not match expected "/relation_embedders,/related_dummies,/third_levels".

Why does fos_user.feature fail? My guess is bad table name?

An exception occurred while executing 'INSERT INTO User (username, username_canonical, email, email_canonical, enabled, salt, password, last_login, confirmation_token, password_requested_at, roles, id, fullname) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' with params ["dummy.user", "dummy.user", "dummy.user@example.com", "dummy.user@example.com", 0, null, "$2y$13$h11wei5V1h8MHUrolsKhCuJo9DgkEDsl62wQVVTJYiTYuncjzRPL6", null, null, null, "a:0:{}", 1, "Dummy User"]:
SQLSTATE[42601]: Syntax error: 7 ERROR:  syntax error at or near "User"
LINE 1: INSERT INTO User (username, username_canonical, email, email...
                    ^

Debug instructions:

docker run --name postgres -e POSTGRES_PASSWORD=api-platform -d postgres:9.6.5-alpine

# tests/Fixtures/app/config/config.yml
 doctrine:
     dbal:
-        default_connection: '%default_connection%'
+        default_connection: 'postgres'
@@ -28,10 +28,10 @@ doctrine:
             postgres:
                 driver:   'pdo_pgsql'
-                dbname:   'api_platform_test'
+                dbname:   'postgres'
                 user:     'postgres'
-                password: ''
-                host:     'localhost'
+                password: 'api-platform'
+                host:     '172.17.0.2' # docker inspect

sroze

Seems very weird that Doctrine isn't in charge of that

sroze · 2017-11-06T10:58:07Z

src/Bridge/Doctrine/Orm/Filter/DateFilter.php

@@ -185,7 +191,7 @@ protected function addWhere(QueryBuilder $queryBuilder, QueryNameGeneratorInterf
            ));
        }

-        $queryBuilder->setParameter($valueParameter, new \DateTime($value));
+        $queryBuilder->setParameter($valueParameter, $type->convertToDatabaseValue(new \DateTime($value), $queryBuilder->getEntityManager()->getConnection()->getDatabasePlatform()));


What if $type is null?

I had the condition before but removed it. Indeed Type::getType will throw if not found (in getDateType).

What I can do:

test for hasType

Use what we had before when null

Though, this method will only be called when the Type is available in the DOCTRINE_DATE_TYPES constant. Therefore, it's not really useful because it'll never be null (those are all defined by default).

sroze · 2017-11-06T11:00:29Z

features/doctrine/date_filter.feature

+  Scenario: Get collection filtered by date that is not a datetime
+    Given there is "30" dummydate objects with dummyDate
+    When I send a "GET" request to "/dummy_dates?dummyDate[after]=2015-04-28"
+    Then the response status code should be 200


You don't test anything in here if you don't test the content of the response. Therefore, I don't understand what is this test trying to prove

If I run this test on my database without the patch it fails to status code 500. This is just to ensure a non-regression.

And it's totally useless on sqlite haha.

Does it affects MySQL or Postgres? it it is the case, maybe should we also run the Behat test suite with such DBMS.

As long as they have two types for dates (ie date vs datetime) yes. And yes I think we should.

I came across this when using a date on an index (it's weird to keep time on an index imo).

soyuka · 2017-11-06T11:16:14Z

Seems very weird that Doctrine isn't in charge of that

Indeed it surprised me as well but according to what I quoted from the docs the behavior is correct no?

soyuka · 2017-11-08T17:09:09Z

tests/Fixtures/app/config/config.yml

-        driver:                        'pdo_sqlite'
-        path:                          '%kernel.cache_dir%/db.sqlite'
-        charset:                       'UTF8'
+        default_connection: '%default_connection%'


I made an attempt with %env(DEFAULT_CONNECTION)% which would've been cleaner IMO but because this is an alias it doesn't work, see symfony/symfony#21609

Why don't you use different environments with multiple config files instead (config_test.yml, config_test_mysql.yml, config_test_pgsql)? Then you just have to set the APP_ENV environment variable in your Travis config instead of doing cp!

would this mean that I'd have to copy the whole configuration file 3 times?

Nop:

config_test.yaml: the whole configuration +

doctrine: dbal: driver: 'pdo_sqlite' path: '%kernel.cache_dir%/db.sqlite' charset: 'UTF8'

config_test_mysql.yaml:

imports: - { resource: config_test.yaml } doctrine: dbal: driver: pdo_mysql path: ~ dbname: 'api_platform_test' user: 'travis' password: '' host: 'localhost'

etc.

soyuka · 2017-11-24T13:20:41Z

All green :D.

ping @sroze @dunglas @meyerbaptiste would love some review on this.

I've simplified the pull request:

the numeric_fitler.feature has a big diff because the old ones were put in search_filter.feature and I wrote new ones for numeric filters.
tests run on postgres, mysql and sqlite (added a JSON should be deep equal to feature to compare without taking the order into consideration).
the only code change is to add the doctrine Type to setParameter and to skip invalid values in filters (which is our policy with Doctrine Filters instead of throwing).

We could argue that there is a BC break where id=9.99 will be ignored, but IMHO it's a bug and would be silly to count it as break.

Let me know your thoughts!

dunglas

Some minor comments, then 👍 for me.

dunglas · 2017-11-27T09:49:06Z

tests/Fixtures/app/config/config_mysql.yml

+imports:
+    - { resource: config_test.yml }
+
+doctrine:


It should use environment variables to allow to run test locally (same for Postgres).

dunglas · 2017-11-27T09:49:57Z

tests/Fixtures/app/console

-$env = $input->getParameterOption(array('--env', '-e'), getenv('SYMFONY_ENV') ?: 'dev');
-$debug = getenv('SYMFONY_DEBUG') !== '0' && !$input->hasParameterOption(array('--no-
A93C
debug', '')) && $env !== 'prod';
+$env = $input->getParameterOption(array('--env', '-e'), $_SERVER['APP_ENV'] ?? null ?: 'test');
+$debug = ($_SERVER['SYMFONY_DEBUG'] ?? null) !== '0' && !$input->hasParameterOption(array('--no-debug', '')) && $env !== 'prod';


$_SERVER['SYMFONY_DEBUG'] ?? 0 ?

dunglas · 2017-11-27T09:50:13Z

tests/Fixtures/app/console

@@ -13,8 +13,8 @@ use Symfony\Component\Console\Input\ArgvInput;
 use Symfony\Component\Debug\Debug;

 $input = new ArgvInput();
-$env = $input->getParameterOption(array('--env', '-e'), getenv('SYMFONY_ENV') ?: 'dev');
-$debug = getenv('SYMFONY_DEBUG') !== '0' && !$input->hasParameterOption(array('--no-debug', '')) && $env !== 'prod';
+$env = $input->getParameterOption(array('--env', '-e'), $_SERVER['APP_ENV'] ?? null ?: 'test');


$_SERVER['SYMFONY_DEBUG'] ?? 'test'?

Test multiple dbms

meyerbaptiste · 2017-12-01T15:23:09Z

tests/Fixtures/app/AppKernel.php

+        $environment = $this->getEnvironment();
+
+        // patch for behat not supporting %env(APP_ENV)% in older versions
+        if ($appEnv = $_SERVER['APP_ENV'] ?? null && $appEnv !== $environment) {


Should not it be if (($appEnv = $_SERVER['APP_ENV'] ?? null) && $appEnv !== $environment) {?

Sorry, fixed by #1524.

Use database type conversion in Filters (test postgres+mysql)

soyuka added the [RFR] Ready for Review label Nov 6, 2017

sroze suggested changes Nov 6, 2017

View reviewed changes

soyuka commented Nov 8, 2017

View reviewed changes

soyuka force-pushed the fix/doctrine-type-where branch 4 times, most recently from f6c0f7b to 9e7d479 Compare November 9, 2017 10:48

soyuka added the bug label Nov 9, 2017

soyuka force-pushed the fix/doctrine-type-where branch 2 times, most recently from c5aaf1b to 9528880 Compare November 9, 2017 14:55

soyuka removed the [RFR] Ready for Review label Nov 9, 2017

soyuka changed the title ~~Use type conversion in DateFilter~~ WIP: Use database type conversion in Filters Nov 9, 2017

soyuka changed the title ~~WIP: Use database type conversion in Filters~~ WIP: Use database type conversion in Filters (test postgres+mysql) Nov 9, 2017

soyuka force-pushed the fix/doctrine-type-where branch 3 times, most recently from c8262c6 to 620d25d Compare November 9, 2017 17:21

soyuka changed the base branch from 2.1 to master November 10, 2017 11:26

soyuka force-pushed the fix/doctrine-type-where branch 9 times, most recently from e503023 to dba1cea Compare November 13, 2017 09:40

soyuka changed the title ~~WIP: Use database type conversion in Filters (test postgres+mysql)~~ Use database type conversion in Filters (test postgres+mysql) Nov 13, 2017

soyuka force-pushed the fix/doctrine-type-where branch 2 times, most recently from 76297af to b5780eb Compare November 13, 2017 10:08

soyuka changed the base branch from master to 2.1 November 23, 2017 16:54

soyuka force-pushed the fix/doctrine-type-where branch from 4bf9863 to 5bf30c4 Compare November 23, 2017 17:01

soyuka mentioned this pull request Nov 23, 2017

Refactor SearchFilter on an identifier or with composite identifier #1515

Closed

soyuka force-pushed the fix/doctrine-type-where branch 14 times, most recently from ff46042 to a586b2e Compare November 24, 2017 13:14

soyuka force-pushed the fix/doctrine-type-where branch 2 times, most recently from ad52ada to c76edc0 Compare November 24, 2017 14:46

soyuka added the [RFR] Ready for Review label Nov 24, 2017

dunglas requested changes Nov 27, 2017

View reviewed changes

Add doctrine type to where in filters

c25b81b

Test multiple dbms

soyuka force-pushed the fix/doctrine-type-where branch from c76edc0 to c25b81b Compare November 27, 2017 13:22

soyuka requested review from meyerbaptiste and sroze November 27, 2017 13:49

soyuka merged commit acb6ee7 into api-platform:2.1 Nov 28, 2017

soyuka deleted the fix/doctrine-type-where branch November 28, 2017 09:19

F62E meyerbaptiste reviewed Dec 1, 2017

View reviewed changes

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#1480 from soyuka/fix/doctrine-type-where

d5040bc

Use database type conversion in Filters (test postgres+mysql)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use database type conversion in Filters (test postgres+mysql) #1480

Use database type conversion in Filters (test postgres+mysql) #1480

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!