Skip to content

fix: Avoid locale-dependent NumberFormat in IntegerSchema#5140

Open
Mattias-Sehlstedt wants to merge 1 commit intoswagger-api:masterfrom
Mattias-Sehlstedt:integer-schema-parse-negative-numbers
Open

fix: Avoid locale-dependent NumberFormat in IntegerSchema#5140
Mattias-Sehlstedt wants to merge 1 commit intoswagger-api:masterfrom
Mattias-Sehlstedt:integer-schema-parse-negative-numbers

Conversation

@Mattias-Sehlstedt
Copy link
Copy Markdown
Contributor

Pull Request

Thank you for contributing to swagger-core!

Please fill out the following information to help us review your PR efficiently.


Description

This is a revival of #4178.

NumberFormat.getInstance().parse(Integer.toString(-1)) always throws exception in the following locales: ar· ·ckb· ·he· ·ks· ·pa_Arab· ·ur· ·fa· ·ps· ·uz_Arab· ·eo· ·et· ·fi· ·fo· ·gsw· ·ksh· ·lt· ·nb· ·nn· ·rm· ·se· ·sv· (see https://bugs.openjdk.java.net/browse/JDK-8189097)

To see this in action, try running the following code:

import java.text.NumberFormat;
import java.text.ParseException;
import java.util.Locale;

public class NumberFormatIsBroken {
    public static void main(String[] args) throws ParseException {
        Locale.setDefault(new Locale("no"));
        NumberFormat.getInstance().parse(Long.toString(-1));
    }
}

It should be valid to ignore locale for IntegerSchema, since we are not conducting any parsing based upon the locale (since we are doing Integer.parseInt() and Long.parseLong(), without considering the Number casted that we instantiate).

The locale is mainly used for different decimal representations and thousand separators, but these already fail silently in the current implementation. This since the Number casted is properly read and populated. E.g., 2 000 to 2000, but we then do Integer.parseInt() with 2 000, which will fail.

Fixes: #4223

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • ♻️ Refactor (non-breaking change)
  • 🧪 Tests
  • 📝 Documentation
  • 🧹 Chore (build or tooling)

Checklist

  • I have added/updated tests as needed
  • I have added/updated documentation where applicable
  • The PR title is descriptive
  • The code builds and passes tests locally
  • I have linked related issues (if any)

Screenshots / Additional Context

Co-authored-by: jhannes <jhannes@users.noreply.github.com>

import static org.testng.Assert.*;

public class IntegerSchemaTest {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If it is of interest I could add the tests first in a PR to showcase the current behavior, and then do a PR afterwards that changes the implementation (but also showcases that the behavior is the same).

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.

bug: IntegerSchema returns null for negative values for some locales

1 participant