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

Compatibility issue with MySQL2 v3.9 #17141

Open
3 of 6 tasks
YamazakiNorihito opened this issue Mar 4, 2024 · 6 comments
Open
3 of 6 tasks

Compatibility issue with MySQL2 v3.9 #17141

YamazakiNorihito opened this issue Mar 4, 2024 · 6 comments

Comments

@YamazakiNorihito
Copy link

YamazakiNorihito commented Mar 4, 2024

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

Since the introduction of the typeCast option to the execute method in MySQL2 version 3.9 (see node-mysql2/pull/2398), we have encountered a problem with Sequelize implementations that assume a query method type cast, resulting in "Invalid Date" errors We have encountered a problem with "Invalid Date" errors in Sequelize implementations that assume typecasting of query methods.

Reproducible Example

const { Sequelize, DataTypes, QueryTypes } = require('sequelize');

const sequelize = new Sequelize('contacts', 'local', 'password', {
  host: 'localhost',
  dialect: 'mysql',
  timezone: '+09:00',
});

const Call = sequelize.define('Call', {
  callAt: {
    type: DataTypes.DATE,
    allowNull: false,
  },
  type: {
    type: DataTypes.INTEGER,
    allowNull: false,
  },
}, {
  timestamps: false,
  tableName: 'calls',
});

(async () => {
  try {
    const call = await sequelize.query(
      'SELECT id, callAt FROM calls WHERE type = $1 ORDER BY callAt DESC LIMIT 1',
      {
        bind: [1],
        type: QueryTypes.SELECT,
      }
    );
    console.log(call);
  } catch (error) {
    console.error('Error during the database query:', error);
  }
})();

Here is the link to the SSCCE for this issue:

What do you expect to happen?

I expect the callAt field to retrieve a Datetime value, indicating the specific date and time of the call. The expected output for the query should look like this:

  Executing (default): SELECT id, callAt FROM calls WHERE type = 1 ORDER BY callAt DESC LIMIT 1
  [ { id: 104138353, callAt: 2024-02-20T18:05:13.000Z } ]

What is actually happening?

  Executing (default): SELECT id, callAt FROM calls WHERE type = ? ORDER BY callAt DESC LIMIT 1
  [ { id: 104138353, callAt: Invalid Date } ]

image

Environment

  • Sequelize version: 6.37.1
  • Node.js version: 18
  • If TypeScript related: TypeScript version:
  • Database & Version: Mysql & 8
  • Connector library & Version: mysql2 & 3.9.2

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I will need guidance.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as "+1" will be removed.

@YamazakiNorihito YamazakiNorihito added pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet type: bug labels Mar 4, 2024
@wellwelwel
Copy link

If you think there's something that could be done from MySQL2 side, please ping me.

@YamazakiNorihito
Copy link
Author

Following a more detailed examination of the issue at hand, I'd like to provide additional insights that may help pinpoint the underlying cause and suggest directions for a potential solution. My analysis has focused on the handling of DATETIME data types within the registerMySqlDbDataTypeParsers function, highlighting a couple of key areas:

  • Handling Binary Data: The process attempts to convert binary-formatted DATETIME data directly into a string using the value.string(); method. However, the string method provided by mysql2's binary_parser.js is primarily designed for converting general binary data into strings with specific encoding. This method may not be ideally suited for DATETIME data, which is encoded in a specialized binary format.

  • Format of DATETIME Data: The encoding for DATETIME data is such that each component (year, month, day, hour, minute, second) is represented by specific bytes or groups of bytes, rather than being encoded as a regular string. For instance, the year is represented in two bytes, and the month in one byte, as numerical data. This specialized format can be seen in the source code of the mysql2 library.

  • Cause of Error: Directly converting DATETIME binary data to a string using value.string(); can lead to misinterpretation of the binary data as it doesn't account for the unique encoding of DATETIME components. This approach is likely causing the "Invalid Date" errors observed.

@ephys ephys removed the pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet label Mar 17, 2024
@ephys
Copy link
Member

ephys commented Mar 17, 2024

Thank you for the thorough analysis of the issue, we'll look into it

@ephys
Copy link
Member

ephys commented Mar 17, 2024

@wellwelwel Would it be a good idea to add functions for packet.readDateTimeString, packet.readDateTime and packet.readTimeString in wrap? We could then call that in our handling of DATETIME

Also, it's really nice to see you in our repo ready to help!

Note to self: dev branch is https://github.com/sequelize/sequelize/tree/ephys/17141

@wellwelwel
Copy link

wellwelwel commented Mar 18, 2024

Thanks, @ephys. I really appreciate your comment 🙋🏻‍♂️✨


About improve typeCast:

Would it be a good idea to add functions for packet.readDateTimeString, packet.readDateTime and packet.readTimeString in wrap?

This is an old TODO from MySQL2:

I'm not sure about developing new features for it.


For other side:

The issue #17116, plus your changes in 17141 brought me a different angle on this issue.
So, I'll try to reproduce it using only MySQL2 (no Sequelize) first.


Goals

  • Analyze what Sequelize receives/expects/sends from or for MySQL2 using a previous version
  • Analyze what Sequelize receives/expects/sends from or for MySQL2 using the version 3.9.2 or higher

@ephys
Copy link
Member

ephys commented Mar 18, 2024

I definitely get not wanting to add features if typeCast is going to be phased out.
If .string() is meant to output the datetime string, then fixing that behavior would fix the issue on our end.

If it's not meant to output the datetime string, I don't see another way to solve this than either having a separate method that produces that string, or by us implementing it ourselves based on the contents of the Buffer

Good luck with figuring out the best solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants