Fix !!timestamp resolution for years 0000-0099 - #775
Closed
sarathfrancis90 wants to merge 1 commit into
Closed
Conversation
Date.UTC() applies JavaScript's legacy two-digit-year remap (0-99 map to 1900-1999), so resolveYamlTimestamp built the wrong year and the normalization guard then rejected valid low four-digit years such as 0001-01-01, leaving them as plain strings. As a side effect a Date in years 1-99 dumped as an explicit !!timestamp and threw on reload under YAML11_SCHEMA. Restore the intended year with setUTCFullYear() before the calendar validity check; passing month/day re-validates the leap day against the real year, so invalid dates (e.g. 0001-02-29) are still rejected.
Member
|
I understand the problem, but I don't like the implementation with a duplicated check for the year subrange. IMHO, it's worth creating a helper function makeUtcDate (year, month, day, hour = 0, minute = 0, second = 0, fraction = 0) {
const date = new Date(Date.UTC(year, month, day, hour, minute, second, fraction))
// Date.UTC() treats years 0..99 as 1900..1999. Restore the parsed YAML year
// before validating calendar normalization, e.g. reject 0001-02-29.
date.setUTCFullYear(year, month, day)
return date
}That's more readable and "straight". Tests are bloated. It's worth just adding a couple of good samples to the existing one ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While round-tripping
Datevalues throughYAML11_SCHEMAI hit a case where a date in years 1–99 throws on reload:And loading a valid low-year timestamp directly silently yields a string instead of a
Date:Root cause:
resolveYamlTimestampbuilds the date withDate.UTC(year, …), which applies JavaScript's legacy two-digit-year remap (0–99 → 1900–1999). The Date is then off by ~1900 years, so the calendar-normalization guard (getUTCFullYear() !== year) rejects the value and it falls back to a plain string. Duringdumpthat same rejection forces an explicit!!timestamptag onto the scalar, which then can't resolve on load — hence the throw.Fix: restore the intended year with
setUTCFullYear(year, month, day)before the validity check. Passing month/day re-applies the day in the context of the real year, so invalid calendar dates like0001-02-29are still rejected.Tested with
npm test(all green). Extendedtest/core/tags/timestamp.test.mjswith low-year load cases, a leap day (0004-02-29), rejection of an invalid low-year date, and a dump → load round-trip.