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

Change the way types for prepared statement parameters are calculated #1407

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sidorares
Copy link
Owner

@sidorares sidorares commented Oct 11, 2021

Currently types are inferred based on what is in the input (Date -> mysql date, js number -> mysql double, everything else - string ). This used to work ok until mysql server version 8.0.22

Calling execute('SELECT * from foo limit ?' , [1]) results in Incorrect arguments to mysqld_stmt_execute error. The type of the parameter server expects ( LONGINT ) is incompatible to what is actually sent ( DOUBLE ).

The fix is to use parameters column definitions returned from the previous prepare() call and use types from there instead of inferring type from the provided input

@sidorares
Copy link
Owner Author

ref #1239

@sidorares
Copy link
Owner Author

@vlasky when you have time can you try this brach to see if it fixes Incorrect arguments to mysqld_stmt_execute error for you?

It's a potentially backwards incompatible change in some scenarios, I'm thinking to release it as a major version bump. Maybe alpha release initially

@kimf
Copy link

kimf commented Apr 27, 2022

Hello @sidorares
For what it's worth, I had the exact same issue as mentioned in #1239 and tried this branch with great success.

@mlucic
Copy link

mlucic commented Nov 23, 2022

Any updates on this?

@sidorares
Copy link
Owner Author

sidorares commented Nov 24, 2022

@mlucic unfortunately no progress here (but its quite high in my priority)

The way I want to implement this currently is different from this PR though.

The plan is:

  1. Introduce a "typed parameter" helpers which would hold the value you pass together with type information. Not sure about the exact api, but it might look like this:
const mediumintparam = mysql.TypedParameter.MEDIUMINT(123);
const floatparam =  mysql.TypedParameter.FLOAT(123);

conn.execute('select ?+? as sum', [mediumintparam, floatparam]);

when used via type helper the type sent with COM_EXECUTE is always what you specify

  1. when untyped js value is passed we have 2 options: infer from current value ( string / date / number / null / non-null object ) or use response from COM_PREPARE. The problem "type hint from the server" is that its not always accurate ( for example, there are scenarios when the server has no way of telling parameter type - select ?+? as sum ). I'll need to think when this hint might be useful and when not ( any feedback is appreciated! ), ideally we need a matrix where there is js type on one axis, COM_EXECUTE type hint on another axis mapped to what the driver will actually use for type

step 1) can be done now and would help solving current edge cases with a bit of manually added typings ( as in mysql types metadata in users code, not typescript )

after that we can add step 2 to make default map work with no surprises most of the time. Mysql is quite capable of casting back to a correct value on a server side, in earlier versions of this driver all .execute() parameters were converted to a string ( sometimes its even more efficient, "1" takes 2 bytes over the wire where double precision number is 8 )

example table:

Mysql type hint \ JS type string number bigint Date
LONGLONG VAR_STRING VAR_STRING VAR_STRING DATETIME
VAR_STRING VAR_STRING DOUBLE VAR_STRING DATETIME

I'm starting to think simple "always send (parameter.toString()) as VAR_STRING" unless the type is explicitly specified by user" might be actually the best behaviour

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.

3 participants