Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSRangeErrorException - "Invalid timezone name!"] Fix Intl.DateTimeFormat bug with normalizing timeZone value #571

Closed
wants to merge 10 commits into from
Closed

[JSRangeErrorException - "Invalid timezone name!"] Fix Intl.DateTimeFormat bug with normalizing timeZone value #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7991f8c
Fix normalizing timeZone function
anton-patrushev Aug 13, 2021
f5ace46
Merge branch 'main' into fix-intl-date-time-format
anton-patrushev Aug 13, 2021
23616f7
Add test for Int.DateTimeFormat API
anton-patrushev Aug 25, 2021
692ef46
Merge branch 'main' into fix-intl-date-time-format
anton-patrushev Aug 25, 2021
0846d3b
Tweak test case
anton-patrushev Sep 22, 2021
c31fe89
Merge branch 'main' into fix-intl-date-time-format
anton-patrushev Sep 22, 2021
b68946c
re-implement timezone normalizing algorithm
anton-patrushev Sep 24, 2021
bfb4de8
fix new algorithm issues
anton-patrushev Sep 24, 2021
b483991
Merge branch 'main' into fix-intl-date-time-format
anton-patrushev Sep 24, 2021
39953b3
add test case to ensure case insensetivity works correctly
anton-patrushev Sep 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions android/intltest/java/com/facebook/hermes/test/HermesInstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,46 @@ public void testDateFormat() {
}
}

@Test
public void testDateTimeFormat() {
try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
rt.evaluateJavaScript(
new StringBuilder()
.append("var date = new Date('2021-08-13T14:00:00Z');\n")
.append("var formattedDate = Intl.DateTimeFormat('en-US', {\n")
.append("timeZone: 'America/New_York',\n")
.append("day: 'numeric',\n")
.append("month: 'numeric',\n")
.append("hour: 'numeric',\n")
.append("minute: 'numeric'\n")
.append("}).format(date);\n")
.toString());

String result = rt.getGlobalStringProperty("formattedDate");
assertThat(result).isEqualTo("8/13, 10:00 AM");
}
}

@Test
public void testDateTimeFormatCaseInsensitivity() {
try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
rt.evaluateJavaScript(
new StringBuilder()
.append("var date = new Date('2021-09-24T22:00:00Z');\n")
.append("var formattedDate = Intl.DateTimeFormat('en-US', {\n")
.append("timeZone: 'AmeRiCa/new_YORK',\n")
.append("day: 'numeric',\n")
.append("month: 'numeric',\n")
.append("hour: 'numeric',\n")
.append("minute: 'numeric'\n")
.append("}).format(date);\n")
.toString());

String result = rt.getGlobalStringProperty("formattedDate");
assertThat(result).isEqualTo("9/24, 6:00 PM");
}
}

@Test
public void testCaseConversion() {
Locale defaultLocale = Locale.getDefault();
Expand Down
30 changes: 5 additions & 25 deletions lib/Platform/Intl/java/com/facebook/hermes/intl/DateTimeFormat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,10 @@ private void initializeDateTimeFormat(List<String> locales, Map<String, Object>
if (JSObjects.isUndefined(timeZone)) {
timeZone = DefaultTimeZone();
} else {
String normalizedTimeZone = normalizeTimeZoneName(JSObjects.getJavaString(timeZone));
if (!isValidTimeZoneName(normalizedTimeZone)) {
throw new JSRangeErrorException("Invalid timezone name!");
try {
timeZone = mPlatformDateTimeFormatter.normalizeValidTimeZone(timeZone.toString());
} catch (JSRangeErrorException error) {
throw error;
Comment on lines +249 to +250
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just remove this try/catch, it doesn't do anything.

}
}
mTimeZone = timeZone;
Expand Down Expand Up @@ -378,28 +379,7 @@ private void initializeDateTimeFormat(List<String> locales, Map<String, Object>
mHourCycle = hc;
}
}

private String normalizeTimeZoneName(String timeZoneName) {
// https://tc39.es/ecma402/#sec-case-sensitivity-and-case-mapping
// Note that we should convert only upper case translation in ASCII range.
StringBuilder normalized = new StringBuilder(timeZoneName.length());
int offset = 'a' - 'A';
for (int idx = 0; idx < timeZoneName.length(); idx++) {
char c = timeZoneName.charAt(idx);
if (c >= 'a' && c <= 'z') {
normalized.append((char) (c - offset));
} else {
normalized.append(c);
}
}

return normalized.toString();
}

private boolean isValidTimeZoneName(String timeZone) {
return mPlatformDateTimeFormatter.isValidTimeZone(timeZone);
}


@DoNotStrip
public DateTimeFormat(List<String> locales, Map<String, Object> options)
throws JSRangeErrorException {
Expand Down
2 changes: 1 addition & 1 deletion lib/Platform/Intl/java/com/facebook/hermes/intl/IPlatformDateTimeFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ String getDefaultCalendarName(ILocaleObject<?> mResolvedLocaleObject)

String getDefaultNumberingSystem(ILocaleObject<?> localeObject) throws JSRangeErrorException;

boolean isValidTimeZone(String timeZone);
String normalizeValidTimeZone(String timeZone) throws JSRangeErrorException;

String[] getAvailableLocales();
}
23 changes: 21 additions & 2 deletions lib/Platform/Intl/java/com/facebook/hermes/intl/PlatformDateTimeFormatterAndroid.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,16 @@
import java.util.Date;
import java.util.Locale;
import java.util.TimeZone;
import java.util.Optional;
import java.util.List;
import java.util.Arrays;
import java.util.function.Predicate;

public class PlatformDateTimeFormatterAndroid implements IPlatformDateTimeFormatter {
private DateFormat mDateFormat = null;

private List<String> allAvailableTimeZones = Arrays.asList(TimeZone.getAvailableIDs());

@Override
public String format(double n) {
return mDateFormat.format(new Date((long) n));
Expand Down Expand Up @@ -204,8 +210,21 @@ else if (needTime)
}

@Override
public boolean isValidTimeZone(String timeZone) {
return TimeZone.getTimeZone(timeZone).getID().equals(timeZone);
public String normalizeValidTimeZone(final String timeZone) throws JSRangeErrorException {
Optional<String> normalizedValidTimeZone = allAvailableTimeZones.stream()
.filter(new Predicate<String>() {
@Override
public boolean test(String tz) {
return tz.equalsIgnoreCase(timeZone);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is incorrect. https://tc39.es/ecma402/#sec-case-sensitivity-and-case-mapping only allows mapping/folding ascii a-z <-> A-Z. equalsIgnoreCase is probably applying additional locale-specific comparisons.

Copy link
Author

Choose a reason for hiding this comment

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

To be honest I don't find it critical. It's strange requirement. But ok, that's how it is.
Any way, I believe my changes will allow map A-Z to a-z and vice versa and it shouldn't be critical issue. I'm finding not working America/New_York (e.g. timezones from IANA TZ db in its original look) more important & crucial.

So I will prefer to leave it as it's right now (since it works, but with such small issues). I guess it will good enough to add test suites which will cover this case sensitivity issue in the future. And fix it.

I will try to do my best and fix it according to ECMA standards and back with result.

Copy link

@andreialecu andreialecu Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You just need to bring this back:

https://github.com/facebook/hermes/pull/571/files#diff-bd6622e75c91b32a4ac2f2a7f9ad2dc1e6c9ffa72c71e78070f3fc57c8d4f4e3L382-L397

Screenshot 2021-10-14 at 20 03 01

Instead of using the built-in case insensitive comparison methods.

}
}).findFirst();

if(!normalizedValidTimeZone.isPresent()) {
String errorMessage = timeZone + " is invalid timeZone";
throw new JSRangeErrorException(errorMessage);
}

return normalizedValidTimeZone.get();
}

@Override
Expand Down
24 changes: 22 additions & 2 deletions lib/Platform/Intl/java/com/facebook/hermes/intl/PlatformDateTimeFormatterICU.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,17 @@
import java.text.AttributedCharacterIterator;
import java.util.ArrayList;
import java.util.Date;
import java.util.Optional;
import java.util.List;
import java.util.Arrays;
import java.util.function.Predicate;

public class PlatformDateTimeFormatterICU implements IPlatformDateTimeFormatter {
private DateFormat mDateFormat = null;

@RequiresApi(api = Build.VERSION_CODES.N)
private List<String> allAvailableTimeZones = Arrays.asList(TimeZone.getAvailableIDs());

@RequiresApi(api = Build.VERSION_CODES.N)
@Override
public String format(double n) {
Expand Down Expand Up @@ -269,8 +276,21 @@ public void configure(

@RequiresApi(api = Build.VERSION_CODES.N)
@Override
public boolean isValidTimeZone(String timeZone) {
return TimeZone.getTimeZone(timeZone).getID().equals(timeZone);
public String normalizeValidTimeZone(final String timeZone) throws JSRangeErrorException {
Optional<String> normalizedValidTimeZone = allAvailableTimeZones.stream()
.filter(new Predicate<String>() {
@Override
public boolean test(String tz) {
return tz.equalsIgnoreCase(timeZone);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

}
}).findFirst();

if(!normalizedValidTimeZone.isPresent()) {
String errorMessage = timeZone + " is invalid timeZone";
throw new JSRangeErrorException(errorMessage);
}

return normalizedValidTimeZone.get();
}

@RequiresApi(api = Build.VERSION_CODES.N)
Expand Down