On Wed, 16 Sep 2026 10:03:14 GMT, Timofei Fedotov <[email protected]> wrote:
>> 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
done
>> src/java.naming/share/classes/javax/naming/ldap/Rfc2253Parser.java line 134:
>>
>>> 132: // IndexOutOfBoundsException instead of the documented
>>> 133: // InvalidNameException.
>>> 134: if (rdn.size() == 0) {
>>
>> `javax.naming.ldap.Rdn` is a public exported class which means that it can
>> be overridden by (external/application) sub-classes. The `size()` method is
>> public too (and thus can be overridden). I think it would be better to avoid
>> calling this method here and instead we should probably call a
>> package-private method which returns the same detail.
>>
>> So I think introducing something like the following new package-private
>> method in `Rdn` class and then calling `rdn.numAttributes()` here might be
>> better:
>>
>>
>> diff --git a/src/java.naming/share/classes/javax/naming/ldap/Rdn.java
>> b/src/java.naming/share/classes/javax/naming/ldap/Rdn.java
>> --- a/src/java.naming/share/classes/javax/naming/ldap/Rdn.java
>> +++ b/src/java.naming/share/classes/javax/naming/ldap/Rdn.java
>> @@ -248,6 +248,15 @@ void sort() {
>> }
>> }
>>
>> + /**
>> + * {@return the number of type/value mappings contained in this Rdn}
>> + * This method is same as {@link #size()}, except that it cannot be
>> + * overridden by sub-classes.
>> + */
>> + final int numAttributes() {
>> + return this.entries.size();
>> + }
>> +
>
> Good point. Since Rdn can be subclassed and size() can be overridden, relying
> on it from the parser could make the validation depend on subclass behavior.
>
> So, I'll add a package-private final numAttributes() method.
done
>> 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
done
>> src/java.naming/share/classes/javax/naming/ldap/Rfc2253Parser.java line 136:
>>
>>> 134: if (rdn.size() == 0) {
>>> 135: throw new InvalidNameException(
>>> 136: "Invalid name: \"" + name + "\" (empty RDN)");
>>
>> Nit - instead of saying "empty RDN", it might be better if the error message
>> said "no attribute type/value mapping present" since the `Rdn` class and
>> method javadoc already refers to that term.
>
> Okay
done
>> test/jdk/javax/naming/ldap/Rdn/EmptyRdnTest.java line 33:
>>
>>> 31: * ("returns the ... type" / "returns the ... value", never an
>>> 32: * exception other than InvalidNameException at construction
>>> time).
>>> 33: * @run main EmptyRdnTest
>>
>> Given the kind of tests being run in this main method, I think it will be
>> better to convert this to a junit test and run each of them as a separate
>> test method.
>>
>> While at it, instead of explicitly referring to the test class name here,
>> jtreg allows the usage of the `${test.main.class}` placeholder for test
>> actions (you can find examples in existing tests in the JDK). Using that
>> placeholder prevents the chances of (copy/pasted) incorrect test class name
>> references which can then result in launching a completely unrelated test
>> (and that can sometimes go unnoticed).
>
> Okay, I'll update the test.
done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4025287905
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4025287322
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4025288570
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4025286656
PR Review Comment: https://git.openjdk.org/jdk/pull/32648#discussion_r4025285645
