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.

Sign up for GitHub

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

Jump to bottom

Replace JSON_EXTRACT utilisation with JSON_VALUE #1897

Open
Angelinsky7 opened this issue Mar 20, 2024 · 5 comments · May be fixed by #1899
Open

Replace JSON_EXTRACT utilisation with JSON_VALUE #1897

Angelinsky7 opened this issue Mar 20, 2024 · 5 comments · May be fixed by #1899
Assignees
@lauxjpn
Labels
type-bug
Milestone
9.0.0-preview.2

Comments

@Angelinsky7
Copy link

Angelinsky7 commented Mar 20, 2024 •
edited

Steps to reproduce

Run the custom project to see it crashing
PomeloBugJsonExtract.zip

The issue

In some places, in the code, JSON_EXTRACT is used for getting value of a JSON field when using projection or any other mean of directly getting data. But when the data is null, JSON_EXTRACT does return the string value "null" instead of the mysql null value, as intended in the spec : https://bugs.mysql.com/bug.php?id=85755
Sadly if you have a Nullable<Datetime> later in ef core when trying to convert it will use the string representation instead of the correct null value and crash with

---> System.FormatException: Couldn't interpret value as a valid DateTime: null
         at MySqlConnector.Core.Row.GetDateTime(Int32 ordinal) in /_/src/MySqlConnector/Core/Row.cs:line 336
         at MySqlConnector.MySqlDataReader.GetDateTime(Int32 ordinal) in /_/src/MySqlConnector/MySqlDataReader.cs:line 264
         at lambda_method186(Closure, QueryContext, DbDataReader, ResultContext, SingleQueryResultCoordinator)
         --- End of inner exception stack trace ---
         at lambda_method186(Closure, QueryContext, DbDataReader, ResultContext, SingleQueryResultCoordinator)
         at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.Enumerator.MoveNext()

be aware that the null indication here is not a real null value but instead a string "null" in

https://github.com/mysql-net/MySqlConnector/blob/1d508bcfd74f221e71a892bbb3086a545f1a5451/src/MySqlConnector/Core/Row.cs#L326-L342

The naive solution

If we replace all call with JSON_VALUE we get a correct null value instead of a string and "everything" seems to work correctly

Further technical details

MySQL version: 8.3.0-1.el8
Operating system: Window11
Pomelo.EntityFrameworkCore.MySql version: 8.0.2
@Angelinsky7
Copy link
Author

Angelinsky7 commented Mar 20, 2024 •
edited

As a really naive approach, i replaced all JSON_EXTRACT to JSON_VALUEand everything is working fine.

Test results:
without the changes:

Réussi!  - échec :     0, réussite :    62, ignorée(s) :     0, total :    62, durée : 327 ms - Pomelo.EntityFrameworkCore.MySql.Tests.dll (net8.0)
Échoué!  - échec :     5, réussite :    32, ignorée(s) :     0, total :    37, durée : 1 s - Pomelo.EntityFrameworkCore.MySql.IntegrationTests.dll (net8.0)
Échoué!  - échec : 11437, réussite : 17625, ignorée(s) :   362, total : 29424, durée : 1 m 1 s - Pomelo.EntityFrameworkCore.MySql.FunctionalTests.dll (net8.0)

with my changes:

Réussi!  - échec :     0, réussite :    62, ignorée(s) :     0, total :    62, durée : 386 ms - Pomelo.EntityFrameworkCore.MySql.Tests.dll (net8.0)
Échoué!  - échec :     5, réussite :    32, ignorée(s) :     0, total :    37, durée : 1 s - Pomelo.EntityFrameworkCore.MySql.IntegrationTests.dll (net8.0)
Échoué!  - échec :  7961, réussite : 21096, ignorée(s) :   367, total : 29424, durée : 1 m 11 s - Pomelo.EntityFrameworkCore.MySql.FunctionalTests.dll (net8.0)

i think I've got failed test because of my mysql config (most of my failed test broke because of : System.Security.Authentication.AuthenticationException : Authentication failed because the remote party sent a TLS alert: 'IllegalParameter')...

but i would gladly make a PR... if it's something doable (i just need to know witch branch to target)

@lauxjpn lauxjpn self-assigned this Mar 21, 2024
@lauxjpn lauxjpn linked a pull request Mar 21, 2024 that will close this issue
Open
@lauxjpn
Copy link
Collaborator

lauxjpn commented Mar 21, 2024

@Angelinsky7 Thanks for making us aware of this issue.

We have implemented a workaround/fix for this issue in #1899, which is likely to be backported to 8.0.x.

@lauxjpn lauxjpn closed this as completed Mar 21, 2024
@lauxjpn lauxjpn reopened this Mar 21, 2024
@lauxjpn lauxjpn added this to the 9.0.0-preview.2 milestone Mar 21, 2024
@Angelinsky7
Copy link
Author

@lauxjpn sorry for the noise, i should have read better #1899 ....

Do you think it would be possible to target 8.0.3 for this ? I've got some feature locked because of this. (i would hope not to wait month to have it)
I would be very happy to do it !
Tell me which branch to target and i can try to backport the code of main on it (if it's ok for you)

Thanks !

@lauxjpn
Copy link
Collaborator

lauxjpn commented Mar 23, 2024

Do you think it would be possible to target 8.0.3 for this ?

@Angelinsky7 That is the plan.

I would be very happy to do it !
Tell me which branch to target and i can try to backport the code of main on it (if it's ok for you)

The 8.0.x branch is 8.0-maint (see Dependencies).

This issue is actually a bit more involved than we initially anticipated, because of the differences in how JSON_EXTRACT() and JSON_VALUE() are implemented between MySQL and MariaDB. The fix for 8.0.x (and probably 6.0.x) also should be backwards compatible.

While we are at it, we are going to improve the EF.Functions.JsonExtract() extension method and maybe introduce a EF.Functions.JsonValue() extension method as well.

So the complexities here are the reason for why we haven't merged #1899 yet and are still working on it.

It is probably best in this case here that I complete the works on the the fixes and improvements myself, since I am already in the middle of it. Thanks!

@Angelinsky7
Copy link
Author

@lauxjpn tell me if i can do something to help you, i would be happy to do my part !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lauxjpn lauxjpn

Labels
type-bug
Projects
None yet
Milestone
9.0.0-preview.2
2 participants
@Angelinsky7 @lauxjpn