Skip to content

Reject trailing characters in StringHelper numeric parsing#665

Open
jmestwa-coder wants to merge 2 commits into
apache:masterfrom
jmestwa-coder:strict-stringhelper-numeric-parsing
Open

Reject trailing characters in StringHelper numeric parsing#665
jmestwa-coder wants to merge 2 commits into
apache:masterfrom
jmestwa-coder:strict-stringhelper-numeric-parsing

Conversation

@jmestwa-coder
Copy link
Copy Markdown
Contributor

Summary

Improve StringHelper::toInt and StringHelper::toInt64 to reject malformed numeric input with trailing non-whitespace characters instead of silently accepting partial parses.

Examples of previously accepted input:

  • "123abc" → parsed as 123
  • "514udp" → parsed as 514

This change now requires the full numeric string to be consumed (excluding trailing whitespace).

Changes

  • Added full-consumption validation after std::stoi / std::stoll
  • Preserved support for surrounding whitespace and signed values
  • Switched to LOG4CXX_ENCODE_CHAR for consistent cross-platform LogString handling
  • Added regression tests covering:
    • trailing characters
    • empty input
    • whitespace-only input
    • embedded whitespace
    • signed numbers

Rationale

OptionConverter::toInt already rejects trailing characters using full-string validation. This change aligns StringHelper numeric parsing behavior with the existing strict parsing semantics used elsewhere in the project and avoids silently accepting malformed numeric configuration values.

@swebb2066
Copy link
Copy Markdown
Contributor

As the failing tests show, this PR breaks existing code that expects trailing non-digits to terminate an integer.

I do not believe this PR adds any value to the code base, especially given that OptionConverter::toInt is available for cases where trailing characters are an error.

@jmestwa-coder
Copy link
Copy Markdown
Contributor Author

@swebb2066 , I was treating the behavior difference as an inconsistency, but I understand now that the permissive parsing in StringHelper::toInt is intentional and relied upon by existing code paths.

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.

2 participants