Add .value() method to the typeCast field wrapper

[Parsonswy]

I was a bit crude here and most or less copy pasted the readCodeFor methods that compile the parser and am open to other approaches.

Another option I considered was modifying the compiled parser so that it could be invoked recursively, but I felt that was going to be a bit hacky since wrap() would need to be able to invoke the parser, but the parser is defined using wrap. It would avoid duplicating the switch statement though.

I also included a simple benchmark that compares querying with default the default cast behavior, custom type cast with the current wrapper methods, and type cast using the added .value() method.

These are the results on my machine against a local docker container with mysql 8.3.0.

query raw: AVG 0.0762ms
query typecast.string(): AVG 0.0705ms
query typecast.value(): AVG 0.0677ms

[wellwelwel]

Thanks, @Parsonswy 🚀

There are some Sequelize traces on typeCast + execute that I believe the issues come from MySQL2:

• Invalid Date on TypeCast for execute after 3.9.x wellwelwel/sequelize#1

Maybe these changes can clarify this issue. As soon as possible I intend to check it out in detail 🙋🏻‍♂️

[wellwelwel]

A question: this PR would work as a feat, right?

Regardless of benchmark, do you think it's possible to make these modifications without "keeping" the breaking changes you mentioned in #1446 (comment)?

For context:

• Compatibility issue with MySQL2 v3.9 sequelize/sequelize#17141 (comment)

[sidorares]

I'm a bit confused. What's the difference between added readCodeFor and existing readValueFor ?
nevermind, I was blind :) I can see the difference now. So what you have in your PR is typeCast field.value is a reference to "non-jit" parser function

Also looks like what we currently pass as a second parameter ( "next()" function ) to a typeCast call is what we actually need from value()? Which means that we only need to update documentation

instead of

  typeCast: function (field, next) {
    if (field.type === 'LONGLONG') {
      return field.string() === '1';
    }
    return next();
  },

we can have an example

  typeCast: function (field, getValue) {
    const value = getValue();
    if (field.type === 'LONGLONG') {
      return value === '1';
    }
    return value;
  },

And later we can deprecate typeCast and introduce mapValue which would get value as a parameter:

  mapValue: function (field, value) {
    if (field.type === 'LONGLONG') {
      return value === '1';
    }
    return value;
  },

[Parsonswy]

Regardless of benchmark, do you think it's possible to make these modifications without "keeping" the breaking changes you mentioned in #1446 (comment)

I don't think I understand the question. By "breaking change" I meant invoking typeCast on binary results at all was a break as there are results which when handled by text protocol with a given implementation of typeCastwill work, but the same implementation would then break if the results are binary parsed. I don't think this PR would have introduced a revert of a breaking change.

Going forward, I don't think it is safe to use the read* methods on the field wrapper though because of that text/bin discrepancy. I think the safe/protocol agnostic way to cast is what @sidorares pointed out.

Also looks like what we currently pass as a second parameter ( "next()" function ) to a typeCast call is what we actually need from value()? Which means that we only need to update documentation.

Hmm. Yes. I think this is totally possible with the current API and I just zeroed in on .value(). Updating the docs to to codify that it is ok to invoke next(); as a way to get a "default" deserialization of the MySQL value and then perform any casting off of that makes sense to me.

I think I can close this PR, but will wait for further comment. Maybe the benchmarks are useful?

[sidorares]

let's close .value() for now and potentially focus on re-packaging it via documentation changes

typeCast benchmarks would be definitely valuable

[max-zu]

Any progress on this ? We need to update mysql2 package due to this security update. But we can't due to this bug sequelize/sequelize#17141

[wellwelwel]

@max-zu

Any progress on this ? We need to update mysql2 package due to this security update. But we can't due to this bug sequelize/sequelize#17141

Although the main issue was opened in Sequelize, the issue seems closer to this #1446 (comment) than to Sequelize.

Unfortunately I haven't been able to reproduce this behavior using MySQL2 only. Any help on this would be appreciated.