[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider (original) (raw)

Langer, Christoph christoph.langer at sap.com
Fri Dec 8 14:39:31 UTC 2017

Hi Rob,

a little style nit: Is it really a good idea to use import java.util.*; in src/java.naming/share/classes/com/sun/jndi/ldap/LdapCtxFactory.java? I thought one is supposed to only use qualified exports nowadays (with all the IDE support).

Best regards Christoph

-----Original Message----- From: core-libs-dev [mailto:core-libs-dev-bounces at openjdk.java.net] On Behalf Of Rob McKenna Sent: Mittwoch, 6. Dezember 2017 19:25 To: vyom tewari <vyom.tewari at oracle.com> Cc: Osipov, Michael <michael.osipov at siemens.com>; core-libs-dev at openjdk.java.net Subject: Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

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