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.

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

[NETBEANS-5181] Update NbBundle to read bundle files like JDK9 does (UTF-8 then ISO-8859-1) #2633

Merged
merged 1 commit into from Jan 9, 2021

Conversation

jjazzboss
Copy link
Contributor

@jjazzboss jjazzboss commented Jan 2, 2021

Changed NbBundle to read Bundle files using UTF-1 encoding by default, and
automatically switch to ISO-8859-1 if UTF-1 decoding fails. System property
java.util.PropertyResourceBundle.encoding can alter the behavior
like for PropertyResourceBundle (from JDK9).

@jjazzboss jjazzboss changed the title [NETBEANS-5181] NbBundle only accepts ISO-8859-1 while UTF-8 is defau… [NETBEANS-5181] NbBundle only accepts ISO-8859-1 while UTF-8 is default since JDK9 Jan 2, 2021
@jjazzboss jjazzboss changed the title [NETBEANS-5181] NbBundle only accepts ISO-8859-1 while UTF-8 is default since JDK9 [NETBEANS-5181] Update NbBundle to read bundle files like JDK9 does (UTF-1 then ISO-8859-1) Jan 2, 2021
@jjazzboss
Copy link
Contributor Author

I checked the travis log, only 1 job failed "#8201.21 Test Java modules without nb-javac on Java 14". There are exceptions in several places, no idea which one is fatal, and it does not seem related to the code I've changed. Help is welcome...

@matthiasblaesing
Copy link
Contributor

For the unittest: That job is flaky - I restartet the test.

For the commit:

Please adjust the title (it is UTF-8, not UTF-1). What I'm missing is a reasoning for the possible issues introduced by this change. How probable is it, that some one used a ISO-8859-1 char from the high range (with the highest bit set) and that that character (or this character and the following characters) form a valid UTF-8 sequence?

For example the sequence ˤ encoded as ISO-8859-1 will yield:

À       §
C   0   A   7
1100000010100111

That correctly decodes as UTF-8 to 삧 (Hangul Bbic). So this might introduce regressions.

How does the JDK handle this situation?

@lkishalmi lkishalmi changed the title [NETBEANS-5181] Update NbBundle to read bundle files like JDK9 does (UTF-1 then ISO-8859-1) [NETBEANS-5181] Update NbBundle to read bundle files like JDK9 does (UTF-8 then ISO-8859-1) Jan 3, 2021
@lkishalmi
Copy link
Contributor

A few unittest would be nice.
Well my guess on the regression is that can be so rare, than if noticed, specifying java.util.PropertyResourceBundle.encoding property explicitly, would solve the issue.

@jjazzboss
Copy link
Contributor Author

Regarding the regression risk, my code has exactly the same approach than JDK.

I'll add a few unit tests.

@@ -10,13 +10,12 @@
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* software distributeFd under the License is distributed on an
Copy link
Contributor

Choose a reason for hiding this comment

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

Something got here, needs to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks ok to me. If I remember the number of bundles in the netbeans code base, I think preventing unnessary waste would be good. In that case, two static variables each holding an instance of Utf8ThenIsoCharset, one with acceptOnlyUTF8 and one without it set.

I left two additional inline comments.

platform/openide.util/src/org/openide/util/NbBundle.java Outdated Show resolved Hide resolved
platform/openide.util/src/org/openide/util/NbBundle.java Outdated Show resolved Hide resolved
@matthiasblaesing
Copy link
Contributor

@jjazzboss thank you. I also double checked and this should indeed be save. To get this finished, a bit of work remains:

  • I assume you read the comment at the top of the files you edited. Your change will also be considered a donation to the ASF, so I assume you are the author and agree to this donation.
  • Please squash the commits into one commit and double check the author information. The name provided in the commits is looks good, but there are three different email addresses listed. Please use your personal address for committing.

If that is done, I'll let travis do its work and will then merge.

…lt since JDK9

Change NbBundle to read Bundle files using UTF-1 encoding by default, and
automatically switch to ISO-8859-1 if UTF-1 decoding fails. System property
java.util.PropertyResourceBundle.encoding can can alter the behavior
like for PropertyResourceBundle (from JDK9).
@jjazzboss
Copy link
Contributor Author

@matthiasblaesing Yes I'm ok for the donation. I squashed the commits. Happy to complete my first PR :-)

@matthiasblaesing
Copy link
Contributor

Thank you.

@matthiasblaesing matthiasblaesing merged commit 396d4be into apache:master Jan 9, 2021
@matthiasblaesing matthiasblaesing added this to the 12.3 milestone Jan 9, 2021
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.

None yet

3 participants