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

Queries run with execute don't use the global typeCast function #717

Closed
coreyjv opened this issue Jan 26, 2018 · 8 comments
Closed

Queries run with execute don't use the global typeCast function #717

coreyjv opened this issue Jan 26, 2018 · 8 comments

Comments

@coreyjv
Copy link
Contributor

coreyjv commented Jan 26, 2018

Is this by design or a bug? Ideally I'd like to use prepared statements to absolutely prevent SQL injection attacks but I need to use a custom typecast function because of issues with timezone in #262.

Thanks

@aikar
Copy link
Contributor

aikar commented Jan 29, 2018

Dupe of my report here: #649

See discussion on that issue.

@sidorares
Copy link
Owner

let's continue discussion here @aikar @coreyjv

suppose the data is coming as (number, date, string) field

What should happen if you call field.string() for first field for example? Read it as usual (as number) first, and then return cast as String(value)? What you expect as field.buffer() result for all three? I suspect underlying binary representation ( say, unsigned 32 bit int for first, 8 byte datetime, lengthcoded int prefixed string for second and third? )

If I place typeCast at upper level ( just before returning values, I think this is where it should be) it's much easier to implement but .buffer() and next() won't be available

@coreyjv
Copy link
Contributor Author

coreyjv commented Jan 29, 2018

Personally I only use the typecast because of #262 where it doesn’t seem like mysql2 respects the timezone configuration parameter. If I didn’t have any issues with DATETIME I’d not use typecast.

@coreyjv
Copy link
Contributor Author

coreyjv commented Feb 7, 2018

@sidorares are there any updates on this?

@sidorares
Copy link
Owner

It's still not very obvious to me how to make it compatible between query() and execute()

In query results (almost) all fields are serialized as strings, so what typeCast api suggest is you call field.string() to get that internal string representation first and then use that however you want and cast to some other type

In execute result ( so called "binary protocol") each field type has it's own serialization format - length-prefixed 8/16/24/32/64 bit numbers, 8 byte datetime etc etc.

Execute might convert all this to string first when field.string() used but that would be mean double conversion and seems silly to me. On the other hand most use cases is to convert only small number of fields that are special to user for some reason so maybe not so silly.

wdyt @coreyjv ?

@coreyjv
Copy link
Contributor Author

coreyjv commented Feb 7, 2018

@sidorares like I mentioned in my comment if #262 can be fixed then I don't need this feature.

@sidorares
Copy link
Owner

ok , I'll focus on fixing timezone options and we'll revisit this later

@wellwelwel
Copy link
Collaborator

Closing due to #2398 🎉

Please feel free to ask anything.

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

No branches or pull requests

4 participants