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

Use JSON_VALUE on MySQL platforms #3135

Merged
merged 4 commits into from
Apr 12, 2022

Conversation

andysh-uk
Copy link
Contributor

@andysh-uk andysh-uk commented Mar 22, 2022

This is my attempt to fix #2088.

On MySQL/MariaDB platforms, a JSON string such as "test" (like Bolt stores for field translations) includes the JSON syntax, so a "LIKE" search fails when looking for the first character (e.g. "test" LIKE 't%' never matches because the first character is the quote, not the "t").

MariaDB [(none)]> select JSON_EXTRACT('"test"', '$[0]');
+--------------------------------+
| JSON_EXTRACT('"test"', '$[0]') |
+--------------------------------+
| "test"                         |
+--------------------------------+
1 row in set (0.001 sec)

However JSON_VALUE works as expected:

MariaDB [(none)]> select JSON_VALUE('"test"', '$[0]');
+------------------------------+
| JSON_VALUE('"test"', '$[0]') |
+------------------------------+
| test                         |
+------------------------------+
1 row in set (0.000 sec)

Two bits I need help with on this PR:

  • there is a change to config/packages/doctrine.yaml which I don't know what I need to do (if anything) to automatically apply this on an update
  • I only have access to fairly recent MariaDB, so I have not been able to test this on earlier MySQL servers. Looking at the docs, this function was added to MySQL in 8.0.21 and MariaDB 10.2.3.

Do you know of a way to further limit my restriction to only use JSON_VALUE on MySQL >= 8.0.21 and MariaDB >= 10.2.3, and fall back to JSON_EXTRACT on earlier servers?

What is Bolt's minimum supported version of MySQL and MariaDB?

@andysh-uk
Copy link
Contributor Author

I’ve been thinking about this some more and have a better implementation I think.

JSON_UNQUOTE exists on both MariaDB and MySQL at the same time (from what I can find) as JSON_EXTRACT (which Bolt is already using.)

Therefore a MySQL/MariaDB version that can’t use JSON_UNQUOTE in all likelihood can’t use JSON_EXTRACT either anyway.

Therefore if we wrap JSON_EXTRACT in JSON_UNQUOTE, we should get either an unquoted string, or a JSON array.

Side point: the use of JSON functions in Bolt 4/5 already limits Bolt to a minimum of MySQL 5.7.8 and MariaDB 10.2.3 (I believe.) Is it worth including this in the system requirements doc page, like the minimum PHP version?

Happy to submit a PR for this if the consensus is yes.

@andysh-uk
Copy link
Contributor Author

andysh-uk commented Mar 22, 2022

Also I’m a bit confused with the coding standards feedback.

Why is an “if / elseif / else” construct suggested to be “if / else / if / else” instead?

I’m happy to change it if needed, just genuinely interested to learn why the more verbose format is preferred. Is it because it’s simpler (2x separate if / else’s vs. 1x combined if / elseif / else?)

@bobdenotter
Copy link
Member

bobdenotter commented Mar 23, 2022

Hey @andysh-uk 👋

Why is an “if / elseif / else” construct suggested to be “if / else / if / else” instead?

If you use else if, it's seen as two distinct cases, but if you use elseif, it'll be seen as a proper "else if".. I've taken the liberty of ninja-editing that in.

In the other thread i've suggested a patch for the YAML migration to be added for this.

@bobdenotter
Copy link
Member

bobdenotter commented Mar 23, 2022

Therefore if we wrap JSON_EXTRACT in JSON_UNQUOTE, we should get either an unquoted string, or a JSON array.

That's a fair point.. Would you suggest doing that as a replacement of this PR, or as a followup?

Side point: the use of JSON functions in Bolt 4/5 already limits Bolt to a minimum of MySQL 5.7.8 and MariaDB 10.2.3 (I believe.) Is it worth including this in the system requirements doc page, like the minimum PHP version?

Yes, let's! And we should add the minimum version of SQLite too, which is a bit more fuzzy.. IIRC: "SQLite 3.17 (with JSON1) or 3.38 and up"

Happy to submit a PR for this if the consensus is yes.

As always, would be much appreciated! :-)

@andysh-uk
Copy link
Contributor Author

andysh-uk commented Mar 23, 2022

If you use else if, it's seen as two distinct cases, but if you use elseif, it'll be seen as a proper "else if".. I've taken the liberty of ninja-editing that in.

Perfect, thank you! I can't honestly say I normally use "elseif", I always write it as you say it!

In the other thread i've suggested a patch for the YAML migration to be added for this.

I saw this, that looks great and really easy to add in changes, brilliant!

That's a fair point.. Would you suggest doing that as a replacement of this PR, or as a followup?

Leave this with me for another day or two, ideally I'd like to replace this PR as the changes in this effectively increase the MySQL version requirement to v8, whereas if my thoughts on JSON_UNQUOTE work the same way, it'll keep the version requirement the same as it is now.

I've converted the PR to a draft for now.

As always, would be much appreciated! :-)

No worries, just created a PR on Github for this.

@andysh-uk andysh-uk marked this pull request as draft March 23, 2022 18:27
@bobdenotter
Copy link
Member

Leave this with me for another day or two, ideally I'd like to replace this PR as the changes in this effectively increase the MySQL version requirement to v8, …

That's a good point. Yeah, we shouldn't bump the minimum required version, without bumping the major of Bolt itself. In other words: with Bolt 6, at the earliest. :-)

I'll keep an eye on this PR, for when it's good-to-go!

@bobdenotter
Copy link
Member

Hey @andysh-uk. Not to rush you or anything, but I was curious if you had an update on this one? :-)

@andysh-uk
Copy link
Contributor Author

Hey @bobdenotter 👋🏽

I did have some trouble when I looked at this initially, it was complaining about an issue with the lexing/parsing (expecting a comma but getting a closing bracket.)

I've come back to it with fresh eyes today, and I think I've cracked it.

I'll get my branch updated with my fix (it's currently only in my personal dev site) and update the PR tomorrow.

@andysh-uk andysh-uk marked this pull request as ready for review April 12, 2022 05:14
@andysh-uk
Copy link
Contributor Author

Hi @bobdenotter,

I think this is ready to go.

I've manually patched my personal website on the staging environment - the "Beginning with" search on my lyrics page is what relies on this functionality.

I've assumed this will go into 5.1.8, so I've used that version number in the migrations file, but obviously you can adjust as necessary.

@bobdenotter bobdenotter changed the base branch from master to 5.1 April 12, 2022 10:23
@bobdenotter bobdenotter changed the base branch from 5.1 to master April 12, 2022 10:23
Copy link
Member

@bobdenotter bobdenotter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @andysh-uk 👍

@bobdenotter bobdenotter merged commit deb9cb4 into bolt:master Apr 12, 2022
bobdenotter added a commit that referenced this pull request May 30, 2022
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 this pull request may close these issues.

Setcontent with a LIKE at the beginning of the string doesn't match
2 participants