My first open source contribution (2) — Working through the review

PR #1267 review → PR #1268

The review of the email validation fix from part 1 suggested a few things to address in the next change: test whitespace other than NBSP, and check that email addresses with an ASCII space inside quotes still passed. When adding the same check to idn-email, the reviewer suggested extracting a shared method.

idn-email is JSON Schema's format for internationalized email addresses. It covers addresses containing non-ASCII characters, such as the ü in münchen@example.com. My first PR had only changed email, so idn-email still accepted an NBSP at the beginning of an address. I opened PR #1268 to fix that and address the review comments.

The same check, different inputs

I moved the whitespace check from EmailFormat into Strings.containsNonAsciiWhitespace() and had both EmailFormat and IdnEmailFormat call it. Rejecting whitespace in idn-email must not also reject letters like ü, so I added a test to make sure münchen@example.com, which contains U+00FC (ü), still passed. To choose additional whitespace cases, I used AI to organize the reference material and compare the results of Character.isWhitespace() and Character.isSpaceChar().

In Java 21, 28 characters return true from at least one of these methods. The table below shows the cases relevant to these tests.

Character isWhitespace() isSpaceChar() What to check
U+0020 · SPACE true true Leave ASCII spaces to the existing validator
U+00A0 · NBSP false true The original bug; already covered by a test
.. .. .. ..
U+2003 · EM SPACE true true An additional Unicode whitespace case
U+3000 · IDEOGRAPHIC SPACE true true An additional Unicode whitespace case

NBSP adds space between characters without allowing a line break there. It looks like whitespace on screen, but Java's isWhitespace() returns false for it. The Character.isWhitespace() documentation lists U+2007 and U+202F as exclusions too. To reject those characters as well, I had to include isSpaceChar() in the check.

Testing spaces inside quotes also required a change to how the test input was built. The existing code constructed a JSON string by adding quotes around the email address. With "joe bloggs"@example.com as the input, the quotes inside the address made that JSON invalid. I replaced the string concatenation with JsonMapperFactory.getInstance().writeValueAsString(email) to serialize it properly.

Whitespace cases I had added to only one test class

I added U+2003 and U+3000 to EmailFormatTest, but the new IdnEmailFormatTest covered only NBSP. The reviewer pointed out that both formats used the same whitespace check and should be tested with the same inputs. I added the two cases to IdnEmailFormatTest as well. The test converts each value in @ValueSource to a character, prepends it to an email address, and checks for a validation error.

IdnEmailFormatTest.java

@ParameterizedTest
@ValueSource(ints = { 0x2003, 0x3000 })
void idnEmailWithLeadingUnicodeWhitespaceShouldFail(int codePoint) {
    String whitespace = new String(Character.toChars(codePoint));
    List<Error> messages = validateIdnEmail(whitespace + "name@email.com");
    assertFalse(messages.isEmpty(), "idn-email with a leading non-ASCII whitespace character should be invalid");
}

There was also a suggestion to combine the test helpers, validateEmail() and validateIdnEmail(), but I left them separate. The whitespace logic was already shared; I would combine the test helpers if more formats needed the same checks. I also left the stream in place rather than replacing it with a loop, unless performance measurements showed it was a bottleneck. I explained what I changed and what I left in place in my reply.

When the reviewer removed the whitespace check and ran the tests, all three cases—NBSP, U+2003, and U+3000—failed.

Whitespace inside quotes still left an open question. The new check scans the entire string for Unicode whitespace, so it rejects not only an NBSP before an email address but also one inside quotes, as in "joe<NBSP>bloggs"@example.com. Here, <NBSP> marks the position of the invisible character; it is not literal text in the address. The reviewer noted that an address like this may be allowed by the internationalized email rules. Handling that distinction would require tracking whether the character was inside quotes, so it was not part of this fix. The test allowing ASCII spaces also remained only in EmailFormatTest.