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

Spanner.date generates an invalid Spanner DATE value for dates before 1000AD #1653

Closed
viralpickaxe opened this issue Jun 23, 2022 · 3 comments · Fixed by #1654
Closed

Spanner.date generates an invalid Spanner DATE value for dates before 1000AD #1653

viralpickaxe opened this issue Jun 23, 2022 · 3 comments · Fixed by #1654
Assignees
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@viralpickaxe
Copy link
Contributor

Spanner.date does not generate Spanner compatible ISO dates for years before 1000AD, for example:
Spanner.date("954-01-01").toJSON() => 954-01-02 (instead of 0954-01-02)

This causes an INVALID_ARGUMENT query error

@viralpickaxe viralpickaxe added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jun 23, 2022
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Jun 23, 2022
@asthamohta
Copy link
Contributor

Hi @viralpickaxe, thanks for bringing this to our attention. I feel this code might be a breaking change for someone who is expecting 954-01-02 and has designed their code that way so what I can do is add another new function toIsoStandardJSON() with your code. Would that be okay?

@viralpickaxe
Copy link
Contributor Author

viralpickaxe commented Jun 30, 2022

Hi @asthamohta, this seems like a pretty bad duct tape solution to a bug that is obviously a mistake rather than intended behaviour, see below the official node.js and java spanner SDK docs:

[NodeJS] SpannerDate - toJSON()

Returns the date in ISO date format. YYYY-[M]M-[D]D

[Java] Struct SpannerDate (4.0.0) - ToString()

Returns the ISO 8601 representation (yyyy-MM-dd) of the date represented by this instance.

Sure this may be a breaking change, but adding a new confusing second serialiser (that javascript doesn't use by default) seems like the wrong answer here, instead of bumping a version

@asthamohta
Copy link
Contributor

Okay, that sounds fair @viralpickaxe. I'll merge your PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants