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

[Daniel Fuchs]

Hi Rob,

This is not a code I know well - but here are a couple
of comments:

LdapCtxFactory.java:

168         NamingException ne = new NamingException();
232         NamingException ne = new NamingException();

Creating an exception is somewhat costly as it records the
backtrace in the Throwable constructor. I don't know if
it's an issue there but I thought I'd mention it. It might
be better to create the exception only if you're actually
going to throw it.

  186         } catch (Exception e) {
  187             ne.setRootCause(e);
  188         }

This will catch the NamingException thrown at line 173
and set it at the cause of another NamingException that
has no message. Maybe it would be better to have two
catch clauses:

  186         } catch (NamingException e) {
  187             throw e;
  186         } catch (Exception e) {
                  ... transform it to NamingException ...

In getDnsUrls:

  198         if (env.containsKey(LdapCtx.DNS_PROVIDER)
  199                 && env.get(LdapCtx.DNS_PROVIDER) != null
  200                 && !env.get(LdapCtx.DNS_PROVIDER).equals(""))

I'd suggest to simplify this and make a single lookup:

              String providerClass = env.get(LdapCtx.DNS_PROVIDER);
              if (providerClass != null && !providerClass.isEmpty()) {
                  ...
                  Class<?> cls = Class.forName(providerClass, true, cl);

best regards,

-- daniel


On 01/02/17 20:05, Rob McKenna wrote:
> New webrev with updated test here:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.03/
>
>     -Rob
>
> On 26/01/17 04:03, Rob McKenna wrote:
>> Ack, apologies, thats what I get for rushing. (which I suppose I'm doing
>> again) That code was actually supposed to be in the getDnsUrls method.
>> Updated in place:
>>
>> http://cr.openjdk.java.net/~robm/8160768/webrev.02/
>>
>> I'll add a couple of tests for the SecurityManager along with some input
>> checking tests too.
>>
>>     -Rob
>>
>> On 25/01/17 05:50, Daniel Fuchs wrote:
>>> Hi Rob,
>>>
>>> I believe you should move the security check to before
>>> the class is actually loaded, before the call to
>>>  171             List<String> urls = getDnsUrls(url, env);
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> On 25/01/17 17:44, Rob McKenna wrote:
>>>> I neglected to include a security check so I've cribbed the one from
>>>> OBJECT_FACTORIES (NamingManager.setObjectFactoryBuilder()) - see:
>>>>
>>>> http://cr.openjdk.java.net/~robm/8160768/webrev.02/
>>>>
>>>> Note, this wraps the SecurityException in a NamingException. Presumably
>>>> its better to throw something than simply leave the default value in
>>>> place, but feedback appreciated!
>>>>
>>>>    -Rob
>>>>
>>>> On 25/01/17 04:31, Rob McKenna wrote:
>>>>> Hi folks,
>>>>>
>>>>> I'm looking for feedback on this suggested fix for the following bug:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-816076
>>>>> http://cr.openjdk.java.net/~robm/8160768/webrev.01/
>>>>>
>>>>> This is something that has come up a few times. Basically in certain
>>>>> environments (e.g. MS Active Directory) there is a dependence on
>>>>> DNS records to provide a pointer to the actual ldap server to be used
>>>>> for a given LdapCtx.PROVIDER_URL where the url itself simply points to the
>>>>> domain name.
>>>>>
>>>>> This fix add a new Ldap context property which allows a user to specify a
>>>>> class (implementing BiFunction) which can perform any necessary extra steps
>>>>> to derive the ldap servers hostname/port from the PROVIDER_URL.
>>>>>
>>>>>    -Rob
>>>>>
>>>