On Wed, 16 Sep 2026 10:02:16 GMT, Timofei Fedotov <[email protected]> wrote:
>> src/java.naming/share/classes/javax/naming/ldap/Rdn.java line 169:
>>
>>> 167: entries = new ArrayList<>(DEFAULT_SIZE);
>>> 168: (new Rfc2253Parser(rdnString)).parseRdn(this);
>>> 169: if (entries.isEmpty()) {
>>
>> Hello @Sovtouch, the `Rfc2253Parser.parseRdn()` already has necessary checks
>> for a few other cases of invalid RDN. I think the `parseRdn(...)` should be
>> able to detect this additional case as well and we should throw the
>> `InvalidNameException` from within the `parseRdn(...)` method instead of
>> adding a check at call sites.
>>
>> I see that the `doParse(...)` method of `Rfc2253Parser` has already been
>> updated in this PR to do this check there, so is there a reason why this
>> additional check is required here in the constructor?
>
> Hello! The additional check in the constructor was intended as a defensive
> check against possible future changes in the parser, but since
> `Rfc2253Parser.parseRdn()` is responsible for validating the RDN and
> `doParse()` now rejects an empty RDN, the constructor check is redundant.
>
> I'll remove it and keep the validation in `Rfc2253Parser`.
You could consider adding:
assert !entries.isEmpty();
instead.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4025218056
