-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
TypeCast blows up values that come after it in row #539
Comments
Yes, I see the issue here. When you make a custom typecast for any of the cases in RowDataPacket.prototype._typeCast that invoke the parser and you don't invoke the parser in the custom typecast for it (can you even? I'm not sure) then the rest of the fields will be garbage like you are experiencing. |
@dougwilson I don't think ive seen an example of a custom typecast configuration invoking the parser... |
The example in the README is as follows: connection.query({
sql: '...',
typeCast: function (field, next) {
if (field.type == 'TINY' && field.length == 1) {
return (field.string() == '1'); // 1 = true, 0 = false
}
return next();
}
}); The same as the way I use it in the sample code above, except that it is part of the connection config and not passed as part of the query. |
@tomasikp right, probably because the example is too simplistic and perhaps it wasn't thought of before. Actually, I just looked and it looks like you have to invoke one of the methods on i.e.: function (field, next) {
if (field.type === 'TIMESTAMP') {
field.string(); // Invokes parser and necessary
return '1';
}
return next();
} |
The moral is in your custom typecast function, you must invoke |
touche, will submit a pull request with the proper README change to explicitly state the importance of invoking field.string() |
Sure, though you cannot always invoke |
I see what you mean:
are aliases for
|
Yea. After seeing this I feel like the way custom type casting works right now makes to too easy for someone to shoot themselves in the foot (even calling |
This is exactly what happened to me. typeCast: function (field, next) {
if(field.type=='TIMESTAMP'){
return require('moment')(field.string()).unix();
}
return next();
} but in some cases field.string() would return null, which in turn would cause moment to throw an error. typeCast: function (field, next) {
if(field.type=='TIMESTAMP'){
return field.string()==null?'':require('moment')(field.string()).unix();
}
return next();
} now I understand why, called field.string twice. |
It is prob not enough to just amend the documentation to warn of this possibility; too easy to shoot oneself in the foot and not realize it... |
A fix for this would prob be a breaking change for everyone using custom typeCasting. The callback parameters would prob change to be something like (Original Field, Parsed) so you could parse the original field yourself if you like, or return the parsed value as-is |
It depends on what the maintainers of the library think. The current version in still "alpha" so they may consider it before it becomes "beta" or stable or whatever, not sure. |
We could make a change in About not calling |
Issue #536 reminded me of a problem I ran into recently. In reality, I am typecasting timestamps to unix time, but for the sake of simplicity here i just typecast all timestamps to the string '1' as you can see the values that follow get garbled. I didn't notice this until recently because most of my timestamp values are the last column in a given table in my tables...
Throws:
AssertionError: true == "NaN"
The text was updated successfully, but these errors were encountered: