On Wed, 16 Sep 2026 09:45:51 GMT, Jaikiran Pai <[email protected]> wrote:
>> Timofei Fedotov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add regression test
>
> 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`.
> src/java.naming/share/classes/javax/naming/ldap/Rfc2253Parser.java line 126:
>
>> 124:
>> 125: // RFC 2253: an RDN MUST contain at least one
>> 126: // attributeTypeAndValue. If the outer loop above never ran
>
> I think we should trim down this comment to just the first sentence and
> remove the rest. So something like:
>
>
> // RFC 2253 - RDN must contain at least one attributeTypeAndValue:
> // RelativeDistinguishedName ::= SET SIZE (1..MAX) OF AttributeTypeAndValue
Okay
> src/java.naming/share/classes/javax/naming/ldap/Rfc2253Parser.java line 135:
>
>> 133: // InvalidNameException.
>> 134: if (rdn.size() == 0) {
>> 135: throw new InvalidNameException(
>
> Please indent this to move it "inside" the `if` block.
Okay
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4024768854
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4024776689
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4024775334
