ramanathan1504 commented on PR #4103:
URL: https://github.com/apache/logging-log4j2/pull/4103#issuecomment-4499337496
Hi @DrDrunkenstien-10!
Appreciate your patience!
This is a really fantastic PR. Not only does it fix the documentation as
requested in Issue #4051, but you also made a great catch by adding the
fail-fast validation directly into the constructors to strictly enforce RFC
5424 compliance.
Before we merge this, there are just a few small things we need to wrap up:
1. **Unit Tests:** Since this PR introduces new validation logic that
actively throws `IllegalArgumentException` in the constructors, could you
please add a few unit tests to `StructuredDataMessageTest.java`?
* It would be great to have a "happy path" test (e.g., exactly 32 valid
characters).
* A few "failure path" tests verifying that `IllegalArgumentException` is
correctly thrown for inputs that are null, exceed 32 characters, or contain
forbidden characters (like spaces, `=`, `]`, or `"`).
2. **Minor Typo:** In the `validateKey` method, there is a missing space in
the exception message string concatenation. It currently reads `"characters" +
"and"`, which will print as `charactersand`. Adding a space there would be
perfect!
3. **Changelog Entry:** Could you please add a quick changelog entry for
this update? (If you haven't done this before, Log4j2 uses a specific folder
structure for changelogs, usually under `src/changelog/`—let us know if you
need any guidance on how to generate or format this!).
Thank you again for this excellent contribution. Please feel free to ping
here if you have any questions or need help setting up the tests or changelog!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
