Use string rather than []byte for types.JSON #301
Conversation
This fixes an issue with MySQL that doesn't accept []byte (mapped to binary in MySQL) as a valid type for JSON columns due to the lack of character encoding information.
|
Oh, to replicate this bug, need the following three conditions:
Then, just run the SQLBoiler tests for generated code and Inserts will fail with: Some relevant MySQL bugs on the binary string for JSON column issue. |
|
I saw this as soon as you opened it, apologies for not having replied sooner. The problem is I didn't know what to do with it. It's another one of these "mysql ruins everything" bugs (which are surprisingly frequent). The proper type for JSON in Go is Allow me to elaborate: You have turned on Now mysql happens to have an annoying bug which they should probably actually fix since as reported there's many people who can't import their exported schemas (which is kind of funny). And we have a driver whose job it is to interface with that broken database but when we use the driver in a way that is seemingly normal for the language we're using it breaks. Literally it'd be beneficial for all users of that database if it were fixed in their driver, instead of just in sqlboiler. Lastly, you might ask why I would resist such a seemingly innocuous change to begin with. The main reason is: I don't like making concessions for a single database engine where I don't have to because at the end of the day maybe the JSON column in postgres or mssql must be a So in order to get this change through there's two conditions:
In the case where it breaks the other database engines, we'll have no choice but to use a new JSON type for this. v3 has the ability to replace types and it's nearing release. Until then hopefully you can keep hanging on with your fork. Sorry for making this tough. I bet never in a million years did you imagine this kind of response when you changed just a few characters haha. I feel bad about it :( |
|
Thanks for the thorough explanation. Completely understand your position. I
think the root of the bug is deeper, really the golang sql driver
specification should support a JSON type rather than forcing a conversion
down to []byte earlier in the process. This would allow The MySQL driver to
map JSON differently than Postgres does. Arguably, anything that is a
column type in common SQL engines should be in the specification so drivers
can do the mapping, not clients like SQLBoiler.
Getting that changed though I imagine may be quite difficult, perhaps can
advocate for this through the go-mysql-driver.
Or, as you say, MySQL should fix this itself, but again a hard place to
push for changes.
OK, good to know v3 can allow individuals to make this change. I think as
you suggest, for SQLBoiler we just leave it as is an ill fork in short term
and move to v3 once ready.
…On Mon, Jun 11, 2018 at 11:09 AM Aaron L ***@***.***> wrote:
I saw this as soon as you opened it, apologies for not having replied
sooner. The problem is I didn't know what to do with it. It's another one
of these "mysql ruins everything" bugs (which are surprisingly frequent).
The proper type for JSON in Go is []byte. In the end this sounds like a
mysql driver bug to me.
Allow me to elaborate: You have turned on interpolateParams=true
presumably for performance which already makes me cautious in accepting the
PR because it's not the normal configuration for the mysql driver. However,
this means that instead of using prepared statements and asking the
database to do all the heavy lifting in terms of types and parameter
replacement, the mysql driver library itself is doing it.
Now mysql happens to have an annoying bug which they should probably
actually fix since as reported there's many people who can't import their
exported schemas (which is kind of funny). And we have a driver whose job
it is to interface with that broken database but when we use the driver in
a way that is seemingly normal for the language we're using it breaks.
Literally it'd be beneficial for all users of that database if it were
fixed in their driver, instead of just in sqlboiler.
Lastly, you might ask why I would resist such a seemingly innocuous change
to begin with. The main reason is: I don't like making concessions for a
single database engine where I don't have to because at the end of the day
maybe the JSON column in postgres or mssql must be a []byte (sounded like
we haven't tested, we really need our CI back, ugh). Or worse, maybe a
database engine we're planning to add sqlite or cockroachdb requires it
and one day we'll have to consider reverting (as it's most likely Go
drivers for any database will want []byte not string given the interface
of the json package).
So in order to get this change through there's two conditions:
1. We need to talk to the go-mysql-driver people and ask them if they
can't fix this.
2. We need to test with the above databases to ensure that it's not
going to break anyone.
In the case where it breaks the other database engines, we'll have no
choice but to use a new JSON type for this. v3 has the ability to replace
types and it's nearing release. Until then hopefully you can keep hanging
on with your fork.
Sorry for making this tough. I bet never in a million years did you
imagine this kind of response when you changed just a few characters haha.
I feel bad about it :(
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#301 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAE1GgMWeoTsdyKBDRp8nCmu4Z2cZK-jks5t7ogXgaJpZM4Ue_od>
.
|
|
I'm fairly sure they have reasonable control over conversions these days in the I haven't played around with the driver interfaces in a long while but I do know that these are new and were designed to tackle this exact problem. If I were you I'd definitely raise an issue on the Thanks for being so understanding. I'm going to close this PR for now (like the issue list to stay relatively clean), but if you remember to please update here if there's any movement on the issue in general :) |
|
@aarondl Just to close, the |
|
Around the corner. I plan on making very few changes to the current v3.0.0-rc2 tag. I'm just working on videos to go along with the release before announcing. |
|
That's to say don't wait. You can use it now. I'll be very responsive to any issues during this time. |
This fixes an issue with MySQL that doesn't accept []byte (mapped to binary in MySQL) as a valid type for JSON columns due to the lack of character encoding information.
Ideally, the Golang SQL driver should have an explicit JSON type so that this can be pushed down to the drivers to handle. However, sadly the driver interface doesn't support this and we must collapse to either
[]byteorstring. I believestringshould work across all databases for JSON.