Skip to content

fix(json_schema): stop TIME rejecting fractional seconds - #270

Open
chrikrah wants to merge 1 commit into
dottxt-ai:mainfrom
chrikrah:fix/time-format-secfrac-backslash
Open

chrikrah wants to merge 1 commit into
dottxt-ai:mainfrom
chrikrah:fix/time-format-secfrac-backslash

Conversation

@chrikrah

Copy link
Copy Markdown

No open issue covers this. Found while reading TIME against DATE_TIME.

types.rs:55 writes the secfrac group as (\\.[0-9]+)? inside a Rust raw string, so the emitted regex is a literal backslash followed by any character. DATE_TIME on line 52 has \. and is correct. Four lines apart, one character different.

--- format=time ---      ...([0-5][0-9])(\\.[0-9]+)?(Z)?"
--- format=date-time --- ...([0-5][0-9])(\.[0-9]{3})?(Z|...)?"

It cuts both ways. A valid RFC3339 full-time is rejected, and a string that cannot occur in JSON at all is accepted, because an unescaped backslash is not legal there:

"15:30:00Z"                -> True
"15:30:00.000Z"            -> False    <- valid, rejected
"2024-01-01T15:30:00.000Z" -> True     <- same construct under date-time
"12:34:56\X789"            -> True     <- cannot appear in a JSON string

This compiles to the DFA that constrains generation, so a model asked for format: time cannot emit fractional seconds.

Flagging a behaviour change, because this is not only a typo fix. Applying it flipped an existing assertion: "15:30:00.000" was listed as should-not-match with the comment // missing Z, but (Z)? is optional by design, which lines 44-45 state for DATE_TIME. The old expectation encoded the bug, so that case moves to should-match in both the TIME and the nested TIME block.

$ cargo test
test result: ok. 37 passed; 0 failed

Baseline on 116a7c3 is the same 37. Reverting types.rs and keeping the test gives 36 passed; 1 failed, with Value should match, but does not, in unreachable for: "15:30:00.000Z". cargo fmt --check and cargo clippy --all-targets -- -D warnings both exit 0, and pytest tests/ gives 45 passed, 2 skipped.

I did not run the asv benchmarks or build for a non-x86_64 target.

Related and not fixed here: TIME still accepts only Z, not a numeric offset. That is what #257 fixed for date-time last month and it was not carried across. Happy to fold it in if you want the two together.

@RobinPicard you merged #257. Is the offset worth the same pass?

TIME wrote the secfrac group as (\\.[0-9]+)? inside a Rust raw string, which
is backslash-backslash-dot, so the emitted regex meant a literal backslash
followed by any character. DATE_TIME four lines above has \. and is correct.

Consequences both ways. A valid RFC3339 full-time is rejected:

  "15:30:00Z"     -> True
  "15:30:00.000Z" -> False

and a string that cannot occur in JSON at all is accepted, because an
unescaped backslash is not legal there:

  "12:34:56\\X789" -> True

This regex compiles to the DFA that constrains generation, so a model asked
for format: time cannot emit fractional seconds.

Fixing it flipped an existing assertion. "15:30:00.000" was listed as
should-not-match with the comment // missing Z, but (Z)? is optional by
design, which lines 44-45 state for DATE_TIME. The old expectation encoded
the bug, so that case moves to should-match in both the TIME and the nested
TIME block.
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

Successfully merging this pull request may close these issues.

1 participant