$dateToString aggregation with timezone outputs invalid ISO8601 string #1768
Conversation
…egation with timezone outputs invalid ISO8601 string
NathanQingyangXu
force-pushed
the
JAVA-5044
branch
from
d879fcb to
281c7a3
Compare
driver-core/src/test/functional/com/mongodb/client/model/mql/TypeMqlValuesFunctionalTest.java
Outdated
Show resolved
Hide resolved
driver-core/src/test/functional/com/mongodb/client/model/mql/TypeMqlValuesFunctionalTest.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/client/model/mql/MqlExpression.java
Outdated
Show resolved
Hide resolved
…ypeMqlValuesFunctionalTest.java Co-authored-by: Viacheslav Babanin <[email protected]>
| * Ticket: JAVA-5044 | ||
| */ | ||
| @Test | ||
| void dateAsStringWithDefaultFormat() { |
There was a problem hiding this comment.
This is testing a new API. But, since this is a bugfix, I would expect it to test an existing API, where the test would fail before the prod change, and pass afterward. Is that not the case?
There was a problem hiding this comment.
yeah, correct. The testing would only succeed for v7.1..0+; it would fail for v6, for instance.
There was a problem hiding this comment.
I am not referring to the difference in test outcome based on the server version. The server would write such a test.
I mean, the ticket suggests that we need to make changes in the driver. When we write a regression test (or, all tests actually), the idea is that the test should not work before the change, but should work after. If the test works both before and after the change, the test has no relation to the change and is not testing it. If it fails both times, the change did not fix the issue.
Before the change to the production API, it is not actually possible to write a test that exhibits the bug via our API. So, no error. After the change, we can now write a test that exhibits the bug. But, we block the test from running on the server version necessary for the bug to show up. So things are "green" at first for that version, but after the prod code change, they are "red". This means we introduced a bug.
There was a problem hiding this comment.
If I didn't misunderstand, you said the same testing case would fail or be red on v6, correct? That is true and I understand that is an issue.
| assertExpression( | ||
| utcDateTime.withZoneSameInstant(ZoneId.of("America/New_York")).format(ISO_LOCAL_DATE_TIME), | ||
| date.asString(of("America/New_York")), | ||
| "{'$dateToString': {'date': {'$date': '2025-07-17T18:00:36.052Z'}, " |
There was a problem hiding this comment.
Shouldn't this test value not have a Z at the end? From the Jira:
It tacks on a "Z" to the end of the string by default, which implies that it is in UTC
But here, we are expecting UTC?
There was a problem hiding this comment.
The "Z" in the expected mql belongs to the input value, not the expected output. Expected output comes from utcDateTime.withZoneSameInstant(ZoneId.of("America/New_York")).format(ISO_LOCAL_DATE_TIME) which is in the format of 2025‑07‑17T14:00:36.052 - this is what server returns when timezone is not UTC and format is omitted.
There was a problem hiding this comment.
Thanks for @vbabanin 's explanation. Yeah, the last Mql string parameter is the expected server input, not output.
There was a problem hiding this comment.
I see, thanks. It is the server version assume that blocks the test from running.
| @@ -953,6 +953,15 @@ public MqlString asString(final MqlString timezone, final MqlString format) { | |||
| .append("timezone", toBsonValue(cr, timezone)))); | |||
| } | |||
|
|
|||
| @Override | |||
| public MqlString asString(final MqlString timezone) { | |||
There was a problem hiding this comment.
I believe that we do not need this method, and that it might actually be introducing the bug described in the Jira. That is, the API as it is now intentionally prevents the user from specifying a timezone without also forcing that user to explicitly specify a format, thus preventing the situation from arising.
For reference:
- Implement date expressions #1045 (comment)
- https://jira.mongodb.org/browse/SERVER-74002 (not as closely related)
There was a problem hiding this comment.
thanks for sharing. Yeah, also saw you raising the point in our team slack channel. I have no objection of closing this PR.
driver-core/src/main/com/mongodb/client/model/mql/MqlExpression.java
Outdated
Show resolved
Hide resolved
…n.java Co-authored-by: Maxim Katcharov <[email protected]>
https://jira.mongodb.org/browse/JAVA-5044
The server upstream ticket aims to fix the following bug for the
$dateToStringaggregation expresison output:formatis not specified andtimezoneis not UTC, the string should not end withZ, which would be interpreted as UTC time, not w.r.t. the specified non-UTC zoneThe server bugfix will end up with removing the trailing
Zfrom the output since v7.1.The change is a breaking one so it was not backported to v6. From driver side, no non-testing code logic needs to be changed unless there is some tech debt as in this PR (our
MqlDate#asString(TimeZone, Format)doesn't support optional format, which is a mandatory precondition of the bug reproduction).For this reason, this PR will do the following:
MqlDate#asString(TimeZone)method to accommodate default formatformatfield into MQL when it was provided expecitly or through the originualMqlDate#asString(TimeZone, Format)methodZwould be absent when non-UTC timezone is used(with server range above v7.1)