Skip to content
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

Setcontent with order by causes SQL exception in 4.1 #1971

Open
andysh-uk opened this issue Oct 8, 2020 · 15 comments
Open

Setcontent with order by causes SQL exception in 4.1 #1971

andysh-uk opened this issue Oct 8, 2020 · 15 comments

Comments

@andysh-uk
Copy link
Contributor

Since upgrading to 4.1, a "setcontent" clause with an order by is causing a SQL exception.

Details

Question Answer
Relevant Bolt Version 4.1.0
Install type Composer install
BC Break yes
PHP version 7.4
Web server Nginx
For UX/UI issues Firefox on Mac OS

Reproduction

Bug summary

Including an orderby clause in setcontent is throwing a SQL exception in Bolt 4.1.0 (with latest dependencies installed using composer update.)

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'TEXT)) ASC) dctrn_result) dctrn_table' at line 1").

Specifics

On my home page template, I am using "setcontent" to render the last 6 items of content type "reviews" in descending order based on the "date" field.

My content type:

reviews:
    name: Reviews
    singular_name: Review
    fields:
        name:
            type: text
            label: Person's name who left the review
        body:
            type: redactor
            label: Content of the review
        date:
            type: date
            mode: datetime
            label: Date/time the review was posted
        link:
            type: text
            label: Link to see the review (optional)
    icon_many: "fa:users"
    icon_one: "fa:users"

The twig tag:

{% setcontent reviews = 'reviews' limit 6 orderby '-date' %}

This worked correctly in 4.0.1, but is throwing the following exception in 4.1.0:

An exception has been thrown during the rendering of a template ("An exception occurred while executing 'SELECT COUNT(*) AS dctrn_count FROM (SELECT DISTINCT id_0 FROM (SELECT b0_.id AS id_0, b0_.content_type AS content_type_1, b0_.status AS status_2, b0_.created_at AS created_at_3, b0_.modified_at AS modified_at_4, b0_.published_at AS published_at_5, b0_.depublished_at AS depublished_at_6 FROM bolt_content b0_ LEFT JOIN bolt_field b1_ ON b0_.id = b1_.content_id AND b1_.type IN ('generic', 'email', 'number', 'collection', 'date', 'filelist', 'textarea', 'set', 'data', 'text', 'image', 'html', 'file', 'hidden', 'checkbox', 'select', 'embed', 'templateselect', 'markdown', 'block', 'slug', 'imagelist', 'article', 'color', 'redactor') LEFT JOIN bolt_field_translation b2_ ON b1_.id = b2_.translatable_id WHERE b0_.content_type = ? AND b0_.status = ? AND b1_.name = ? ORDER BY LOWER(CAST(b2_.value AS TEXT)) ASC) dctrn_result) dctrn_table' with params ["reviews", "published", "date"]:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'TEXT)) ASC) dctrn_result) dctrn_table' at line 1").

The database is hosted on MariaDB 10.5.5, PHP 7.4.10.

Steps to reproduce

  • Create a content type as above
  • Create multiple items of that content type, making sure to specify the date
  • Put the twig tag into a template (e.g. the home page template)
  • View the page using the twig template (e.g. the home page)

Expected result

The tag would work, and something like {{ reviews|length }} would show the number of review items added.

Actual result

Exception as described above.

Switching the order by tag to ascending results in the same exception.

Removing the order by tag works but obviously the content is not ordered.

@andysh-uk
Copy link
Contributor Author

Removing the LOWER(CAST(b2_.value AS TEXT)) from the ORDER BY clause (so it is just ORDER BY b2_.value ASC) works correctly.

@andysh-uk
Copy link
Contributor Author

I suspect something in this pull request has introduced this.

For example, this comment references casting b2_.value to TEXT for non-MySQL databases (although my driver in .env is using mysql: DATABASE_URL=mysql://)

@bobdenotter
Copy link
Member

Hmm, it seems to be working fine here, on my Mysql 5.7 setup. I can do this:

    {% setcontent reviews = 'showcases' limit 6 orderby '-date' printquery %}

And the result is:

SELECT content FROM Bolt\Entity\Content content LEFT JOIN content.fields fields_order_1 LEFT JOIN fields_order_1.translations translations_order_1 WHERE content.contentType = :ct0 AND content.status = :status_1 AND fields_order_1.name = :order_1 ORDER BY lower(CAST(translations_order_1.value as BLOB)) DESC

ct0: showcases
status_1: published
order_1: date

Removing the LOWER(CAST(b2_.value AS TEXT)) from the ORDER BY clause (so it is just ORDER BY b2_.value ASC) works correctly.

Where are you removing this? From OrderDirective.php? Flat out removing might work for your specific setup, but that's not a good overall fix.

It seems like Cast itself should be present in mariadb, like in Mysql. Could you try if casting it to something other than TEXT works?

->addOrderBy('lower(CAST(' . $translationsAlias . '.value as CHAR))', $direction); or
->addOrderBy('lower(CAST(' . $translationsAlias . '.value as BLOB))', $direction);

Alternatively, could you check to see that happens if you order on a text field?

    {% setcontent reviews = 'showcases' limit 6 orderby 'name' printquery %}

@Wieter
Copy link
Contributor

Wieter commented Oct 9, 2020

The culprit could be very well the src/Doctrine/Query/Cast.php function.
There I implemented a simple test such that known text fields won't be casted to text if the mysql driver is used, as this in turn throws an error (basically: text can not be casted to text and therefore an error needs to be thrown..).
However, I also see you removed the LOWER function. While getting PostgreSQL feature working I also struggled with this.
Just to eliminate this as candidate, can you maybe replace
LOWER(CAST(b2_.value AS TEXT))
by
CAST(b2_.value AS TEXT)
and see what happens?

A difference between MySQL and MariaDB is that MariaDB uses LONGTEXT, maybe a good direction to start investigating.
https://mariadb.com/kb/en/json-data-type/#:~:text=JSON%20is%20an%20alias%20for,performance%20is%20at%20least%20equivalent.

I wonder what your $backend_driver in src/Doctrine/Query/Cast.php yields, if this is indeed mysql as given in the .env then the cast should have been omitted.

@andysh-uk
Copy link
Contributor Author

Hi @bobdenotter ,

SELECT content FROM Bolt\Entity\Content content LEFT JOIN content.fields fields_order_1 LEFT JOIN fields_order_1.translations translations_order_1 WHERE content.contentType = :ct0 AND content.status = :status_1 AND fields_order_1.name = :order_1 ORDER BY lower(CAST(translations_order_1.value as BLOB)) DESC

This query looks different to mine - in place of translations_order_1, I get b2_. Also my cast is to TEXT, not BLOB.

Might this be because I wasn't using printquery (thank you for teaching me something new!) - I was referencing the SQL query reported by the exception.

Where are you removing this? From OrderDirective.php? Flat out removing might work for your specific setup, but that's not a good overall fix.

No, I've not touched any code. I had copied the query and ran it directly against MariaDB in PhpMyAdmin, replacing the ?s with the actual parameter values. I observed the same error from MariaDB. When I removed the LOWER(CAST(.... from the ORDER BY, the query ran successfully in PhpMyAdmin.

Alternatively, could you check to see that happens if you order on a text field?

The same - I still get the LOWER and CAST to a TEXT field.

@Wieter

It seems as though it's the datatype MariaDB doesn't like. CASTing to CHAR works, but to a TEXT or BLOB results in the same error.

Working:

SELECT b0_.id AS id_0, b0_.content_type AS content_type_1, b0_.status AS status_2, b0_.created_at AS created_at_3, b0_.modified_at AS modified_at_4, b0_.published_at AS published_at_5, b0_.depublished_at AS depublished_at_6 FROM bolt_content b0_ LEFT JOIN bolt_field b1_ ON b0_.id = b1_.content_id AND b1_.type IN ('generic', 'email', 'number', 'collection', 'date', 'filelist', 'textarea', 'set', 'data', 'text', 'image', 'html', 'file', 'hidden', 'checkbox', 'select', 'embed', 'templateselect', 'markdown', 'block', 'slug', 'imagelist', 'article', 'color', 'redactor') LEFT JOIN bolt_field_translation b2_ ON b1_.id = b2_.translatable_id WHERE b0_.content_type = 'reviews' AND b0_.status = 'published' AND b1_.name = 'date' ORDER BY LOWER(CAST(b2_.value AS CHAR)) DESC

Not working:

SELECT b0_.id AS id_0, b0_.content_type AS content_type_1, b0_.status AS status_2, b0_.created_at AS created_at_3, b0_.modified_at AS modified_at_4, b0_.published_at AS published_at_5, b0_.depublished_at AS depublished_at_6 FROM bolt_content b0_ LEFT JOIN bolt_field b1_ ON b0_.id = b1_.content_id AND b1_.type IN ('generic', 'email', 'number', 'collection', 'date', 'filelist', 'textarea', 'set', 'data', 'text', 'image', 'html', 'file', 'hidden', 'checkbox', 'select', 'embed', 'templateselect', 'markdown', 'block', 'slug', 'imagelist', 'article', 'color', 'redactor') LEFT JOIN bolt_field_translation b2_ ON b1_.id = b2_.translatable_id WHERE b0_.content_type = 'reviews' AND b0_.status = 'published' AND b1_.name = 'date' ORDER BY LOWER(CAST(b2_.value AS TEXT)) DESC
SELECT b0_.id AS id_0, b0_.content_type AS content_type_1, b0_.status AS status_2, b0_.created_at AS created_at_3, b0_.modified_at AS modified_at_4, b0_.published_at AS published_at_5, b0_.depublished_at AS depublished_at_6 FROM bolt_content b0_ LEFT JOIN bolt_field b1_ ON b0_.id = b1_.content_id AND b1_.type IN ('generic', 'email', 'number', 'collection', 'date', 'filelist', 'textarea', 'set', 'data', 'text', 'image', 'html', 'file', 'hidden', 'checkbox', 'select', 'embed', 'templateselect', 'markdown', 'block', 'slug', 'imagelist', 'article', 'color', 'redactor') LEFT JOIN bolt_field_translation b2_ ON b1_.id = b2_.translatable_id WHERE b0_.content_type = 'reviews' AND b0_.status = 'published' AND b1_.name = 'date' ORDER BY LOWER(CAST(b2_.value AS BLOB)) DESC

I wonder what your $backend_driver in src/Doctrine/Query/Cast.php yields, if this is indeed mysql as given in the .env then the cast should have been omitted.

Do you know which package this file is in? I'm using a standard Composer install of Bolt.

@andysh-uk
Copy link
Contributor Author

I wonder what your $backend_driver in src/Doctrine/Query/Cast.php yields, if this is indeed mysql as given in the .env then the cast should have been omitted.

I did see that in your code and thought the CAST shouldn’t be there

Thinking more about this (while I’m not at my computer!) I wonder if Doctrine is sniffing the DB version string and switching out the driver name as mariadb instead of mysql? I’m assuming there are other syntax differences that Doctrine needs to handle between the two.

As soon as I can get back to my computer I’ll find the file and check what the property yields.

@andysh-uk
Copy link
Contributor Author

@Wieter, the code in src/Doctrine/Query/Cast.php is not being hit at all. Looking back at @bobdenotter's comment, it is the OrderDirective.php that is causing it.

I believe it is this commit which was introduced with the PostgreSQL functionality - it isn't checking for MySQL and is always including the CAST - compared to the Cast.php which does check for MySQL.

I'm going to submit a PR now, hope this is OK @bobdenotter, it's my first contribution to Bolt 😄

@Wieter
Copy link
Contributor

Wieter commented Oct 12, 2020

A short summary of the findings in PR #1977 (which can be closed, as a different fix should be implemented):

The cause of this problem is the following:

Using composer install an config/packages/doctrine.yaml is created within the users project folder.
In there, a different CAST is loaded into Doctrine ORM DQL, namely: CAST: DoctrineExtensions\Query\Mysql\Cast
This loads the file: ./vendor/beberlei/doctrineextensions/src/Query/Mysql/Cast.php
And this in turn collides with our Cast.php, which is not loaded at all.

More is missing from the doctrine config ( config/packages/doctrine.yaml ), the following lines (and only those) should be present under string_functions:

doctrine:
    orm:
        dql:
            string_functions:
                JSON_EXTRACT: Bolt\Doctrine\Functions\JsonExtract
                JSON_GET_TEXT: Scienta\DoctrineJsonFunctions\Query\AST\Functions\Postgresql\JsonGetText
                CAST: Bolt\Doctrine\Query\Cast

Using a git installation of bolt/core:master, this problem is not present as a correct config/packages/doctrine.yaml is loaded.

@Wieter
Copy link
Contributor

Wieter commented Oct 13, 2020

@bobdenotter Composer install get its config/packages/doctrine.yaml from the bolt/project repository, correct?
As mentioned above, it differs from the bolt/core version. This not only breaks MySQL/MariaDB, but also PostgreSQL as JSON_GET_TEXT is missing in that version.
How to ensure the same contents as in bolt/core? Or maybe it is possible to link to there?

@bobdenotter
Copy link
Member

We can't directly link to it, because once it's been "composer create project"-ed, it's set in stone. There's two things we can do:

  • Add it to overrides in services_bolt.yaml that gets generated by bolt
  • And/or add a check for it in bobdenotter/configuration-notices, instucting people to make the change themselves.

Up until now, it's been a manual task to ensure changes in config/ of bolt/core get propagated to bolt/project, but it's not ideal. Not sure how to fix that, though. I haven't thought of a convenient way to allow them to be the same, yet.

@peterboorsma
Copy link
Contributor

I can confirm this bug (it's problematic because sites in production are affected).

{% setcontent agenda = 'agenda' where { date_to: '>today' } limit 5 orderby 'date_to' %}

SQLSTATE[42000]: Syntax error or access violation: 1064 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 'TEXT)) ASC) dctrn_result) dctrn_table' at line 1").

 Bolt version: 4.1.0

 * Install type: Composer install
 * Database: mysql 5.7.26 - 127.0.0.1 via TCP/IP (with JSON)
 * PHP version: 7.3.12
 * Symfony version: v5.1.7
 * Operating System: Darwin - 19.6.0

@bobdenotter
Copy link
Member

We're updating bolt/project as well as "Configuraton Notices", to make it more visible to people to update doctrine.yaml.

The fix is detailed in this reply, above: #1971 (comment)

@andysh-uk
Copy link
Contributor Author

Thanks @bobdenotter

Is there a way to stop Composer pulling new 4.x releases automatically? I.e. I just did a composer update and got upgraded from 4.0.1 -> 4.1.0.

I would happily expect 4.0.1 -> 4.0.2 to be pulled in automatically as this shouldn't require any changes, but if I was directed to an "upgrade guide" to go from 4.0.x -> 4.1.x, the additional requirements could be documented in that.

@Wieter
Copy link
Contributor

Wieter commented Oct 14, 2020

We can't directly link to it, because once it's been "composer create project"-ed, it's set in stone. There's two things we can do:

  • Add it to overrides in services_bolt.yaml that gets generated by bolt
  • And/or add a check for it in bobdenotter/configuration-notices, instucting people to make the change themselves.

Up until now, it's been a manual task to ensure changes in config/ of bolt/core get propagated to bolt/project, but it's not ideal. Not sure how to fix that, though. I haven't thought of a convenient way to allow them to be the same, yet.

Maybe on the long run these things that are so essential to the proper functioning of bolt/core should indeed be migrated into services_bolt.yaml instead, to ensure that with every upgrade/update/installation the core configuration remains up to date and properly set.
For adding a check in bobdenotter/configuration-notices: this has to be kept up to date as well at all times, and critical things (that work in the development version and tests) might get overlooked in the final release, like happened here.

@bobdenotter
Copy link
Member

@andysh-uk :

Is there a way to stop Composer pulling new 4.x releases automatically? I.e. I just did a composer update and got upgraded from 4.0.1 -> 4.1.0.

Yes, by setting it to "4.0.*" instead of "^4.0".

But, ideally we won't be breaking anything in minor versions, because we're trying to stick to SemVer. We're still getting the hang of that, after the big Four Point Oh release. :-)

@Wieter :

Maybe on the long run these things that are so essential to the proper functioning of bolt/core should indeed be migrated into services_bolt.yaml instead

If we can come up with a reliable way to do so, that'd be nice.. Thinking out loud: It should be able to handle duplicates and overrides (which will happen if people create-project from newer versions. 🤔 💭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants