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 a LIKE at the beginning of the string doesn't match #2088

Closed
andysh-uk opened this issue Nov 1, 2020 · 10 comments · Fixed by #3135
Closed

Setcontent with a LIKE at the beginning of the string doesn't match #2088

andysh-uk opened this issue Nov 1, 2020 · 10 comments · Fixed by #3135
Labels
🐛 tag: bug This is a bug.

Comments

@andysh-uk
Copy link
Contributor

andysh-uk commented Nov 1, 2020

I'm using the equivalent of a setcontent tag in some controller code to match all content records that start with a given letter.

However this doesn't work - it always returns 0 records.

I've narrowed this down in the following code and query:

$contentType = 'lyrics-artists';
$filterParameters['title'] = 'A%';
$matchingContent = $this->boltQuery->getContent($contentType, $filterParameters);
SELECT content FROM Bolt\Entity\Content content LEFT JOIN content.fields fields_title LEFT JOIN fields_title.translations translations_title WHERE content.contentType = :ct0 AND (JSON_EXTRACT(translations_title.value, '$[0]') LIKE :title_1 AND fields_title.parent IS NULL AND fields_title.name = :field_title)

    ct0: lyrics-artists
    title_1: a%
    field_title: title

A content with the title field set to "A Great Big World" does not match in the above query.

If I run JSON_EXTRACT manually in MariaDB, it includes the quotes in the JSON, so the above string begins with "A not A as the LIKE statement is expecting.

SELECT value, JSON_EXTRACT(value, '$[0]') FROM `bolt_field_translation` WHERE translatable_id = 76 
value JSON_EXTRACT
["A Great Big World"] "A Great Big World"

Including the " in the code does indeed return the record I was expecting:

$contentType = 'lyrics-artists';
$filterParameters['title'] = '"A%';
$matchingContent = $this->boltQuery->getContent($contentType, $filterParameters);

I believe the solution is to use JSON_VALUE instead of JSON_EXTRACT - and I'm happy to send a PR, I'm just unsure if this is the right solution (will this break anything elsewhere in Bolt/MySQL?) and also the impact on PostgreSQL - @I-Valchev ?

SELECT value, JSON_EXTRACT(value, '$[0]'), JSON_VALUE(value, '$[0]') FROM `bolt_field_translation` WHERE translatable_id = 76 
value JSON_EXTRACT JSON_VALUE
["A Great Big World"] "A Great Big World" A Great Big World
@andysh-uk
Copy link
Contributor Author

According to the MySQL documentation (at least in v8.0):

The statement SELECT JSON_VALUE(json_doc, path RETURNING type) is equivalent to the following statement:

SELECT CAST(
    JSON_UNQUOTE( JSON_EXTRACT(json_doc, path) )
    AS type
);

Which is another solution, but it looks like using JSON_VALUE is simpler (as JSON_EXTRACT can just be switched out for JSON_VALUE, instead of wrapping it in another function).

@bobdenotter bobdenotter added the 🐛 tag: bug This is a bug. label Nov 2, 2020
@I-Valchev
Copy link
Member

Hi @andysh-uk , I have not tested in on my end, but I agree about the proposed solution.

I do not think it will affect PostgreSQL, because we use its JSON_GET_TEXT instead of JSON_EXTRACT there: https://github.com/bolt/core/blob/master/src/Doctrine/JsonHelper.php#L31
🙂
If you can make a PR and the tests pass, I say let's merge and fix this!

@I-Valchev
Copy link
Member

Hi @andysh-uk , don't wanna push you on this, just wondering if you think there's a good fix for this yet? :-)

@andysh-uk
Copy link
Contributor Author

Hi @I-Valchev, honestly no I haven't.

My concern with introducing JSON_VALUE is having to introduce another change to the doctrine.yaml config file, which would break Bolt on upgrades if this file isn't updated.

Do you know if JsonHelper returns DQL or SQL? If it returns SQL, this shouldn't be a problem.

Also the clause would need to be split to separate MySQL as Sqlite doesn't support JSON_VALUE.

@I-Valchev
Copy link
Member

@andysh-uk afaik, the wrapJsonFunction returns SQL, not DQL (hence why we differentiate between JSON_GET_TEXT and JSON_EXTRACT).

Also the clause would need to be split to separate MySQL as Sqlite doesn't support JSON_VALUE.
Would that mean this bugfix won't work for Sqlite?

Ideally, we'd figure out a fix that works for all DMBSes, or at least MySQL and Sqlite 🤔

@andysh-uk
Copy link
Contributor Author

andysh-uk commented Nov 27, 2020

afaik, the wrapJsonFunction returns SQL, not DQL (hence why we differentiate between JSON_GET_TEXT and JSON_EXTRACT).

Ah OK, that may be easier then. I'll give it a try over the weekend. It was just that both JSON_GET_TEXT and JSON_EXTRACT have handlers in doctrine.yaml so it looked like they could return DQL.

Ideally, we'd figure out a fix that works for all DMBSes, or at least MySQL and Sqlite 🤔

Yeah that'd be ideal, it's hard when different DBMSes do things differently!

I'm not too familiar with SQLite, but reading this page:

The json_extract(X,P1,P2,...) extracts and returns one or more values from the well-formed JSON at X. If only a single path P1 is provided, then the SQL datatype of the result is ..... the dequoted text for a JSON string value ....

So it sounds like SQLite already returns the text not "the text" so it may not be an issue for SQLite.

@andysh-uk
Copy link
Contributor Author

I've just given this a try, and it looks like this is returning DQL, as the code now fails with the following error:

[Syntax Error] line 0, col 242: Error: Expected known function, got 'JSON_VALUE'

So I'm at a bit of an impass.

I can change the function name used in Doctrine/Functions/JsonExtract to JSON_VALUE, however this will break SQLite (I believe).

The right thing is to keep the change I've made in JsonHelper (which uses JSON_VALUE in MySQL) but this will require a new Functions class to handle it, and a change in doctrine.yaml, so it is a breaking change.

@I-Valchev
Copy link
Member

hi @andysh-uk, in the last few weeks @bobdenotter has worked a lot on a new yaml-migrations tool, that will allow us to update the yaml configuration of Bolt projects automatically, hence no breaking changes. We can use that in 4.2 to update the doctrine.yaml.

Does that help with your solution here?

@andysh-uk
Copy link
Contributor Author

andysh-uk commented Mar 17, 2022

Hi @I-Valchev,

Apologies I've been away from Bolt for a bit, but I've come back to the original project where this issue. It is still an issue in 5.1, so I'm re-visiting this.

I've got the changes ready, but I just need to understand how to make the relevant changes in doctrine.yml automatically on an update. Is this something you or @bobdenotter could help me with, or point me in the direction of any documentation?

andysh-uk added a commit to andysh-uk/bolt-core that referenced this issue Mar 22, 2022
@bobdenotter
Copy link
Member

bobdenotter commented Mar 23, 2022

I've got the changes ready, but I just need to understand how to make the relevant changes in doctrine.yml automatically on an update. Is this something you or @bobdenotter could help me with, or point me in the direction of any documentation?

There's not much docs on this, unfortunately.. But, if you take a look at (for example) https://github.com/bolt/core/blob/master/yaml-migrations/m_2021-09-16-doctrine.yaml, it might make it a bit more clear.

So, I think in this case, it should be something like m_2022-03-23-doctrine.yaml:

# Adding JSON_VALUE, see https://github.com/bolt/core/pull/3135

file: packages/doctrine.yaml
since: 5.1.6

add:
    doctrine:
        orm:
            dql:
                string_functions:
                    JSON_VALUE: Bolt\Doctrine\Functions\JsonValue

andysh-uk added a commit to andysh-uk/bolt-core that referenced this issue Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 tag: bug This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants