Force Gregorian calendar in FTP listing timestamp parsers - #406
Conversation
garydgregory
left a comment
There was a problem hiding this comment.
Use JUnit Pioneer to set the default Locale: https://junit-pioneer.org/docs/default-locale-timezone/
| @@ -68,6 +75,25 @@ protected FTPFile nullFileOrNullDate(final FTPFile f) { | |||
| return f; | |||
| } | |||
|
|
|||
| /** | |||
| * The RFC 3659 time stamp is a numeric Gregorian date. Parsing it must not depend on the JVM default locale's calendar, which for e.g. a Thai locale is a | |||
| * Buddhist calendar that would read the year 543 years out. | |||
| */ | |||
| @Test | |||
| void testParseGMTdateTimeWithNonGregorianDefaultLocale() { | |||
| final Locale defaultLocale = Locale.getDefault(); | |||
| try { | |||
| Locale.setDefault(new Locale("th", "TH")); | |||
| final Calendar parsed = MLSxEntryParser.parseGMTdateTime("20100313224553"); | |||
| final GregorianCalendar expected = new GregorianCalendar(TimeZone.getTimeZone("GMT"), Locale.ROOT); | |||
| expected.clear(); | |||
| expected.set(2010, Calendar.MARCH, 13, 22, 45, 53); | |||
| assertEquals(expected.getTimeInMillis(), parsed.getTimeInMillis()); | |||
| } finally { | |||
| Locale.setDefault(defaultLocale); | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| @Test | |||
| void testDefaultPrecision() { | |||
There was a problem hiding this comment.
Use JUnit Pioneer to set the default Locale: https://junit-pioneer.org/docs/default-locale-timezone/
There was a problem hiding this comment.
switched all three locale tests over to junit pioneer's @DefaultLocale. added the test dependency in the pom (version comes from commons-parent, 1.9.1 on Java 8). this also drops the manual setDefault/try-finally, which Copilot flagged as parallel-unsafe.
| void testParseTimestampWithNonGregorianDefaultLocale() throws ParseException { | ||
| final Locale defaultLocale = Locale.getDefault(); | ||
| try { | ||
| Locale.setDefault(new Locale("th", "TH")); |
There was a problem hiding this comment.
Use JUnit Pioneer to set the default Locale: https://junit-pioneer.org/docs/default-locale-timezone/
| void testAbsoluteYearWithNonGregorianDefaultLocale() { | ||
| final Locale defaultLocale = Locale.getDefault(); | ||
| try { | ||
| Locale.setDefault(new Locale("th", "TH")); |
There was a problem hiding this comment.
Use JUnit Pioneer to set the default Locale: https://junit-pioneer.org/docs/default-locale-timezone/
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Forces FTP listing timestamp parsing to use the Gregorian calendar regardless of the JVM default locale, preventing era/year mis-parsing (e.g., Thai Buddhist calendar shifting years by +543).
Changes:
- Make MLSxEntryParser use
Locale.ROOTfor RFC 3659 timestamp parsing. - Force
FTPTimestampParserImpldate formats to use aGregorianCalendar. - Update EnterpriseUnixFTPEntryParser to use a
GregorianCalendarand add regression tests covering non-Gregorian default locales.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/apache/commons/net/ftp/parser/MLSxEntryParser.java | Constructs SimpleDateFormat with Locale.ROOT to avoid non-Gregorian default calendars. |
| src/main/java/org/apache/commons/net/ftp/parser/FTPTimestampParserImpl.java | Forces SimpleDateFormat instances to use GregorianCalendar for parsing. |
| src/main/java/org/apache/commons/net/ftp/parser/EnterpriseUnixFTPEntryParser.java | Uses GregorianCalendar when setting absolute year from listings. |
| src/test/java/org/apache/commons/net/ftp/parser/MLSxEntryParserTest.java | Adds regression test for RFC 3659 parsing under Thai default locale. |
| src/test/java/org/apache/commons/net/ftp/parser/FTPTimestampParserImplTest.java | Adds regression test for numeric-year parsing under Thai default locale. |
| src/test/java/org/apache/commons/net/ftp/parser/EnterpriseUnixFTPEntryParserTest.java | Adds regression test ensuring absolute year is interpreted Gregorian under Thai default locale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final String year = Integer.toString(now.get(Calendar.YEAR)); | ||
| final String timeStampStrPlusYear = timestampStr + " " + year; | ||
| final SimpleDateFormat hackFormatter = new SimpleDateFormat(recentDateFormat.toPattern() + " yyyy", recentDateFormat.getDateFormatSymbols()); | ||
| hackFormatter.setCalendar(new GregorianCalendar()); |
There was a problem hiding this comment.
good point, the formatter being Gregorian wasn't enough on its own. the appended year came from the caller's serverTime, which under a Thai default locale is itself a Buddhist calendar, so it was still 543 out. now reading the year through a Gregorian calendar at the same instant before building the string. added a recent-date test (testParseRecentTimestampWithNonGregorianDefaultLocale) that passes a Buddhist serverTime and fails without this change.
garydgregory
left a comment
There was a problem hiding this comment.
@dxbjavid
Please address my comments as well as Copliot's. Ty!
| void testParseGMTdateTimeWithNonGregorianDefaultLocale() { | ||
| final Locale defaultLocale = Locale.getDefault(); | ||
| try { | ||
| Locale.setDefault(new Locale("th", "TH")); |
There was a problem hiding this comment.
Use JUnit Pioneer to set the default Locale: https://junit-pioneer.org/docs/default-locale-timezone/
|
@garydgregory pushed an update addressing both your note and Copilot's. the three locale tests now use junit pioneer's @DefaultLocale instead of setting the default locale by hand; i added junit-pioneer as a test dependency and the version is inherited from commons-parent (1.9.1, which is the Java 8 line). on Copilot's substantive point: forcing the formatter to a Gregorian calendar wasn't quite enough for the recent-date path, because the year appended there is read from the caller's serverTime, and under a Thai default locale that calendar is itself Buddhist, so the parsed instant was still 543 years out. it now reads that year through a Gregorian calendar at the same instant. i also added a recent-date test that passes a Buddhist serverTime and fails without the change. full build is green, checkstyle clean. |
the FTP listing parsers build their SimpleDateFormat and Calendar objects without an explicit locale, so on a JVM whose default locale carries a non-Gregorian calendar (a Thai Buddhist calendar, or the ja_JP_JP imperial calendar) a server-supplied numeric year is read in the wrong era and the parsed instant ends up centuries out, 543 years under th_TH. this affects MLSxEntryParser.parseGMTdateTime (the RFC 3659 MLSD/MLST time stamp), the shared FTPTimestampParserImpl used by the unix/nt/vms/os400/etc parsers, and EnterpriseUnixFTPEntryParser, which sets the absolute year from a listing into a default-locale Calendar. forcing a Gregorian calendar at each construction site keeps parsing independent of the ambient locale; the added tests parse a listing under a Thai default locale and check the resulting instant, and fail on the current code.
mvn; that'smvnon the command line by itself.