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

[Osipov, Michael]

Hi Rob,

a few comments on the webrev 07:

1. You have changed the semantics of domainName from null to "". This has implications on the StartTlsResponseImpl in regard with cert validation. This has to be discussion with someone who is firm with the STARTTLS command.
2. I miss documentation that says LdapDnsProviderResult can be null in LdapDnsProviderService(). If it is not null, getEndpoints() and getDomainName() must not be null. This uncertainty can easily lead to NPEs.
3. LdapCtxFactory:
+ 		// getDnsUrls(url, env) may throw a NamingException, which there is
+            // no need to wrap.
There is no getDnsUrls anymore
4. What is the purpose of using Void in LdapDnsProvider?


Michael 

> -----Original Message-----
> From: Rob McKenna [mailto:rob.mckenna at oracle.com]
> Sent: Wednesday, December 06, 2017 7:25 PM
> To: vyom tewari
> Cc: core-libs-dev at openjdk.java.net; Osipov, Michael (GS IT PD LD PLM)
> 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
> > >
> >