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

fix(server): switch to JSON_EXTRACT and JSON_UNQUOTE for MySQL/MariaDB. Fixes #13202 #13203

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

spaced
Copy link
Contributor

@spaced spaced commented Jun 17, 2024

fix: use standard JSON_EXTRACT and JSON_UNQUOTE function for list workflow using mariadb. Fixes #13202

--> is only a alias to JSON_EXTRACT() function

(The -> operator serves as an alias for the JSON_EXTRACT()

for some fields it was even wrong (for example it makes no sense to unquote the $.metadata.labels.
mariadb does not support this (see feature request in mariadb: https://jira.mariadb.org/browse/MDEV-13594)

Fixes #13202
Follow-up to #12912

Motivation

be compatible with mariadb

Modifications

  • replace -> with JSON_EXTRACT
  • replace --> with JSON_UNQUOTE(JSON_EXTRACT(..,..)

Verification

locally

@spaced spaced force-pushed the fix-13202 branch 2 times, most recently from f04688e to 051e314 Compare June 17, 2024 15:35
@agilgur5 agilgur5 changed the title fix: use standard JSON_EXTRACT and JSON_UNQUOTE function. Fixes #13202 fix(server): use standard JSON_EXTRACT and JSON_UNQUOTE functions. Fixes #13202 Jun 17, 2024
@agilgur5 agilgur5 changed the title fix(server): use standard JSON_EXTRACT and JSON_UNQUOTE functions. Fixes #13202 fix(server): switch to JSON_EXTRACT and JSON_UNQUOTE functions for MySQL/MariaDB. Fixes #13202 Jun 17, 2024
@agilgur5 agilgur5 changed the title fix(server): switch to JSON_EXTRACT and JSON_UNQUOTE functions for MySQL/MariaDB. Fixes #13202 fix(server): switch to JSON_EXTRACT and JSON_UNQUOTE for MySQL/MariaDB. Fixes #13202 Jun 17, 2024
@spaced spaced marked this pull request as ready for review June 17, 2024 15:50
@agilgur5
Copy link
Contributor

@jiachengxu could you review/double-check this? Thanks again for all your follow-ups ❤️

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

The changes make sense but I wonder if we should introduce additional e2e test suits for other DBs for compatibility

@jiachengxu
Copy link
Member

The change looks good to me, and to Terry's point, I think it makes sense to add e2e tests for DB compatibility

@spaced
Copy link
Contributor Author

spaced commented Jun 20, 2024

@terrytangyuan @jiachengxu should the e2e test with mariab part of this PR or anything else is needed to get merged? It would be also nice if this can be back ported to version 3.5.x.

… list to be compatible with mariadb

Signed-off-by: spaced <[email protected]>
@agilgur5
Copy link
Contributor

agilgur5 commented Jun 23, 2024

It would be also nice if this can be back ported to version 3.5.x.

We'd do that automatically as it's a fix 👍
I'm also release managing 3.5.x right now and so also know to backport it when merged.

should the e2e test with mariab part of this PR or anything else is needed to get merged?

IMO, I think this is fine as-is since it fixes an unintended compatibility regression (although really should be on MariaDB to be compatible, no?). Tests would be great to add, but we pretty rarely change DB queries, I'd be fine if they're added later. We also currently only test MySQL in CI and don't test Postgres either. Adding MariaDB requires a new manifest and profile as well, so it's a non-trivial change (substantially larger than this PR)
@terrytangyuan are you fine with that?

@agilgur5 agilgur5 added this to the v3.5.x patches milestone Jun 23, 2024
@terrytangyuan terrytangyuan merged commit ac9cb3a into argoproj:main Jun 24, 2024
28 checks passed
agilgur5 pushed a commit that referenced this pull request Jul 6, 2024
…riaDB. Fixes #13202 (#13203)

Signed-off-by: spaced <[email protected]>
(cherry picked from commit ac9cb3a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.5.6: Archive workflow sql syntax error with mariadb
4 participants