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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Rob McKenna
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

Reply | Threaded
Open this post in threaded view
|

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

Vyom Tewari
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 ?

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) ?

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();

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
>

Reply | Threaded
Open this post in threaded view
|

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

Rob McKenna
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
> >
>
Reply | Threaded
Open this post in threaded view
|

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

Vyom Tewari
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
>>>

Reply | Threaded
Open this post in threaded view
|

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

Langer, Christoph
In reply to this post by Rob McKenna
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:[hidden email]] On Behalf Of Rob McKenna
Sent: Mittwoch, 6. Dezember 2017 19:25
To: vyom tewari <[hidden email]>
Cc: Osipov, Michael <[hidden email]>; [hidden email]
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
> >
>