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

Support for SQL style string datetimes and timestamps with {dateStrings: true}. #70

Open
rfanth opened this issue Oct 2, 2017 · 2 comments

Comments

@rfanth
Copy link
Contributor

rfanth commented Oct 2, 2017

This package generally works great, but I have run in to a significant problem with the limitations of the JavaScript Date objects used by the package as output for SQL DateTime and TimeStamp values.

The JavaScript Date format cannot represent anything before 1970, even though the SQL Date and DateTime can. Also, TimeStamps can have the value of 0000-00-00 00:00:00. Round tripping in a database application is severely hampered by the fact that the JavaScript Date cannot hold most of the values that MySQL can represent.

The mysql package solved the problem by having a dateStrings: true option, and I think it can be solved similarly here. I already wrote some code to fix it, but I want your opinion on the best way to pass a dateStrings option through to common.js where it is best replaced.

I fixed it by replacing the new Date(...) calls in common.js with calls to two new functions, that can either call new Date(...) and return it, mimicking the old behavior, or build a string out of the value, like one that an sql select statement would return, or that the node mysql package would return if dateStrings: true is set. It works great, and seems to entirely fix the problem, but there is still the matter of relaying the dateStrings option to exports.readMysqlValue in common.js.

Do you have a preferred way to relay dsn.dateStrings (or perhaps options.dateStrings if you prefer) to readMysqlValue in the common.js file? It could be done as a function argument, with a setter in the file, or some other way.

Thanks,

R.F.

@numtel
Copy link
Collaborator

numtel commented Oct 3, 2017

Those are good points on the limitations of the JS Date object. The mysql package probably has an option for 64-bit ints that don't fit in JS numbers either.

The emitter argument of readMysqlValue in common.js receives the zongji instance and then the connection details can be accessed from the ctrlConnection property according to these rules.

You'll want to rename that argument, probably to zongji since it's no longer just for emitting errors. Also, it needs to be renamed where called in rows_event.js. To test, you'll have to modify the relevant type tests to accept different results based on the differing connection details.

@rfanth
Copy link
Contributor Author

rfanth commented Oct 8, 2017

I just added a pull request fixing this #71 . It passes all the tests that I can run with my version of MariaDB, both with and without the dateStrings option. MariaDB does not support the built in JSON objects available in very recent versions of MySQL yet, so I can't run those. The type tests usefully check the code with and without the dateStrings option without modification due to the fact that they interpret symmetrically on both ends of the equality statements.

Your input was really useful here, and without that I think it would have taken much longer to figure out how to read the option cleanly. I wanted others to have the benefit of that, so I put a few useful notes in the code to make it easier for people in the future.

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

2 participants