Skip to content

Conversation

chrisgavin
Copy link
Contributor

👋

This adds a query that looks for Java SimpleDateFormat constructors that are passed patterns containing both "Y" and "M". "Y" represents the ISO 8601 week year, but "M" represents the standard calendar month, so using both in conjunction yields strange results that were probably not intended.

Using "Y" instead of "y" is a particularly easy mistake to make, even with extensive testing, because the majority of the year the formatter will return the correct value. It's only when everyone is having a relaxing holiday break that the latent bug will appear. 😆

This query yields 70 results on LGTM.com projects, with all the results appearing to be true positives.

Some of the format patterns in the results are totally invalid (containing placeholders that are not recognized at all), but many would appear to work correctly until the end of the year, only then returning an incorrect result. Potentially there is room to further expand this query to detect syntactically invalid patterns too, but that would require a bit more sophisticated parsing of the pattern to remove quoted strings.

@chrisgavin chrisgavin added the Java label Jan 8, 2020
@chrisgavin chrisgavin requested a review from a team January 8, 2020 09:43
@chrisgavin chrisgavin force-pushed the suspicious-date-format branch from 006168d to 7185b63 Compare January 8, 2020 13:46
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just needs a change note (and perhaps a test).

@felicitymay
Copy link
Contributor

Would you mind regenerating the qhelp preview? It seems to have expired.

@aschackmull
Copy link
Contributor

I've retriggered CI - that should regenerate the qhelp preview.

Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for including a help file. This looks good. I've made a few suggestions to bring it into line with existing query help.

@@ -0,0 +1,19 @@
/**
* @name Suspicious date format
* @description Some date format patterns don't work as they might seem.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to write descriptions so that they follow the pattern: "Syntax X causes behavior Y".

Possibly this might work here: "Using a data format that includes both 'M' and 'Y' is likely to give unexpected results." ?

Some <code>SimpleDateFormat</code> patterns might not work correctly at the end of the calendar
year, due to use of the <code>Y</code> placeholder (representing the ISO 8601 week year), rather
than <code>y</code> representing the actual year.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly it's worth expanding this overview. Something more like:

The Java SimpleDateFormat class provides many placeholders so that you can define precisely the date format required. However, this also makes it easy to define a pattern that doesn't behave exactly as you intended. The most common mistake is to use the Y placeholder (which represents the ISO 8601 week year), rather than y (which represents the actual year). In this case, the date reported will appear correct until the end of the year, when the "week year" may differ from the actual year.

<example>
<p>
The following example uses the date format <code>YYYY-MM-dd</code>.
On the 30th of December 2019, this code will output "2020-12-30", rather than the intended "2019-12-30".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth being explict about the fix, something like:
"This can be corrected by replacing YYYY-MM-dd with yyyy-MM-dd."

@chrisgavin chrisgavin force-pushed the suspicious-date-format branch from 1797f20 to 484333b Compare January 27, 2020 11:58
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. All LGTM now.

@aschackmull aschackmull merged commit 3745388 into github:master Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants