Skip to content

MATH-1688: Reject malformed grouping separators in ComplexFormat - #330

Open
efegokdemir wants to merge 2 commits into
apache:masterfrom
efegokdemir:codex/MATH-1688-complex-format-comma-validation
Open

efegokdemir wants to merge 2 commits into
apache:masterfrom
efegokdemir:codex/MATH-1688-complex-format-comma-validation

Conversation

@efegokdemir

Copy link
Copy Markdown

Summary

Fixes MATH-1688 by preventing the default ComplexFormat parser from silently accepting malformed grouping separators as part of numeric values.

Changes

  • Disable grouping separators in the default number formats created by ComplexFormat.
  • Add regression coverage for malformed comma placement reported in MATH-1688.
  • Custom NumberFormat instances remain unchanged.

Testing

  • mvn -pl commons-math-legacy -am -Dtest=ComplexFormatTest -Dsurefire.failIfNoSpecifiedTests=false -Djacoco.skip=true test — passed (44 tests).
  • git diff --check — passed.
  • mvn -pl commons-math-legacy -am -DskipTests -Djacoco.skip=true verify — blocked by the repository's SpotBugs plugin, which does not support the JDK 27 class-file version (Unsupported class file major version 71).

Notes

The issue was reproduced on the current upstream branch before the change: malformed inputs such as ,,7+,,,2i and 7,,8 were accepted. The fix is limited to ComplexFormat defaults and does not alter caller-supplied number formats.

AI assistance was used during investigation and implementation; the change was reviewed and validated by the submitter.

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.17%. Comparing base (f554608) to head (4d664da).
⚠️ Report is 350 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #330      +/-   ##
============================================
+ Coverage     86.54%   87.17%   +0.63%     
+ Complexity     9787       89    -9698     
============================================
  Files           532      499      -33     
  Lines         35516    33469    -2047     
  Branches       6194     5834     -360     
============================================
- Hits          30738    29178    -1560     
+ Misses         3518     3165     -353     
+ Partials       1260     1126     -134     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aherbert

Copy link
Copy Markdown
Contributor

This change does not pass the code style checks.

Please run this using a JDK lower than 27 so you can use the default maven goal and pass all the checks. It should pass with:

mvn -pl commons-math-legacy
# if the build is flaky you can increase the test count from the default of 5, e.g
mvn -pl commons-math-legacy -Dsurefire.rerunFailingTestsCount=10

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@efegokdemir

Copy link
Copy Markdown
Author

Applied the requested style correction in commit 3b39724: ComplexFormat declarations now follow the repository Checkstyle declaration order (static field, instance fields, constructors, then helper methods).

Validation with JDK 21.0.12.1:

  • mvn -pl commons-math-legacy: Checkstyle passes; the build remains red only because the existing legacy suite has intermittent numerical/optimization failures.
  • mvn -pl commons-math-legacy -Dsurefire.rerunFailingTestsCount=10: same unrelated suite failure remained after reruns.
  • Focused ComplexFormatTest,ComplexFormatAbstractTest: 44 tests passed.
  • git diff --check: passed.

The correction commit is SSH-signed and DCO-signed. Please re-check the PR when convenient.

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.

3 participants