[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider
vyom tewari
vyom.tewari at oracle.com
Thu Dec 7 15:44:19 UTC 2017
Hi Rob,
Latest code looks good to me.
minor bit.
LdapDnsProviderService.java
Line: 71 , while loop condition is bit complex, we can simplify it little bit as follows.This will make code more readable and easy to understand.
while (iterator.hasNext())
{
LdapDnsProvider p = iterator.next();
result = p.lookupEndpoints(url, env);
if(result != null && !result.getEndpoints().isEmpty()){
break;
}
}
It is just a personal preference you can ignore it if you don't like it.
Thanks,
Vyom
On Wednesday 06 December 2017 11:54 PM, Rob McKenna wrote:
> Thanks Vyom, these should be fixed in:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.07/
>
> Comments inline:
>
> On 06/12/17 22:14, vyom tewari wrote:
>> Hi Rob,
>>
>> Please find below comments.
>>
>> DefaultLdapDnsProvider.java
>>
>> line 35: convert it to for-each code will be more readable.
>>
>> LdapDnsProviderService.java
>>
>> line 21: constant name does not follow Java naming convention, there are
>> other places as well please fix it.
>>
>> getInstance() is already synchronized why you need another "synchronized"
>> block ?
> Ah, neglected to remove the outer synchronized keyword, thanks.
>
>> Line 70: Please use result.getEndpoints().isEmpty() in place of
>> result.getEndpoints().size() == 0
>>
>> LdapDnsProvider.java
>>
>> constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which does
>> nothing, can you explain why LdapDnsProvider() calls LdapDnsProvider(Void
>> unused) ?
> I've copied this pattern from the System.LoggerFinder api and I had
> forgotten what it was for too:
>
> http://www.oracle.com/technetwork/java/seccodeguide-139067.html#7
>
> Section 7.3.
>
>> LdapDnsProviderResult.java
>>
>> Private field domainName & endpoints both can be final.
>>
>> LdapDnsProviderTest.java
>>
>> Fix the tag Order it is not correct. correct order is as follows.
>>
>> /**
>> * @test
>> * @bug 8160768
>> * @summary ctx provider tests for ldap
>> * @modules java.naming/com.sun.jndi.ldap
>> * @compile dnsprovider/TestDnsProvider.java
>> * @run main/othervm LdapDnsProviderTest
>> * @run main/othervm LdapDnsProviderTest nosm
>> * @run main/othervm LdapDnsProviderTest smnodns
>> * @run main/othervm LdapDnsProviderTest smdns
>> */
>>
>> Line 82,83 you don't need System.out.println(e); e.printStackTrace();
> I'm going to leave these in as I've had a few failing tests in the past
> where I've needed to push diagnostic changes like this to determine the
> root cause.
>
> -Rob
>
>> Line 70: you don't need this try block
>>
>> Line 96 : constant name does not follow Java naming convention
>>
>> Line 105: you can use try-with-resources this will avoid some boilerplate.
>>
>> Thanks,
>> Vyom
>>
>> On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:
>>> As this enhancement is limited to JDK10 this frees us up to explore a
>>> different approach.
>>>
>>> http://cr.openjdk.java.net/~robm/8160768/webrev.06/
>>>
>>> In this webrev we're using the Service Provider Interface to load an
>>> implementation of the new LdapDnsProvider class. Implementations of this
>>> class are responsible for resolving DNS endpoints for a given ldap url
>>> should the default implementation be insufficient in a particular
>>> environment.
>>>
>>> Note: if a security manager is installed the ldapDnsProvider
>>> RuntimePermission must be granted.
>>>
>>> -Rob
>>>
More information about the core-libs-dev
mailing list