Thanks folks,
Vyom, I've updated service to be volatile.
On 30/10/18 14:25, Daniel Fuchs wrote:
> Hi Rob,
>
> LdapCtxFactory.java
>
> 187 for (String u : r.getEndpoints()) {
> 188 try {
> 189 ctx = getLdapCtxFromUrl(
> 190 r.getDomainName(), new LdapURL(u), env);
> 191 } catch (NamingException e) {
> 192 // try the next element
> 193 lastException = e;
> 194 }
> 195 }
>
> is a break statement missing after line 190?
>
> If not then can you add a comment explaining why only the last
> context is retained (and returned?)
>
> Alternatively, if a break is indeed missing, these three lines
> could be moved into the for loop above:
>
> 206 // Record the URL that created the context
> 207 ctx.setProviderUrl(url);
> 208 return ctx;
>
> and maybe lines 206-207 could be moved into the
> getLdapCtxFromUrl() method?
>
Yes, you're right, we should return the first successful result.
> LdapDnsProviderService.java:
>
> Why is this class non final? If it can be made final then
> the protected methods should probably become package.
>
Good point, fixed.
> LdapDnsProvider.java:
>
> It is strange to see new APIs with Hashtable in the method
> signature. I understand that our implementation will need
> an Hashtable at some point to call javax.naming.spi.NamingProvider,
> but how likely is it that a clean room implementation of
> a LdapDnsProvider will need to do that?
>
> Maybe we could have Map<?,?> in the signature instead - and
> leave the burden to our implementation - somewhere in ServiceLocator,
> to adapt back to Hashtable where it needs to?
So I've altered the signature of the method to take a Map<?, ?> as
proposed. I've added a getLdapService(String domainName, Map<?,?> environment)
method to ServiceLocator which delegates to the existing method after
conversion. Hopefully this addresses your concerns.
I'll update the CSR accordingly once this review is complete.
-Rob
>
>
> best regards,
>
> -- daniel
>
>
> On 25/10/2018 17:34, Rob McKenna wrote:
> > This recently received CSR approval, so it seems like a good time to pick
> > the codereview up again:
> >
> > http://cr.openjdk.java.net/~robm/8160768/webrev.08/
> >
> > Referencing:
> >
> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html
> >
> > 1) I'm copying the behaviour from
> > LdapCtxFactory.getInitialContext(Hashtable<?,?>) where an empty String is
> > the default value used when the provider url is null.
> >
> > I don't think HostnameChecker (by way of SNIHostname) will accept either
> > empty or null values when doing the comparison.
> >
> > Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
> > appears to pass the String "null" to
> > StartTlsResponseImpl.setConnection(Connection, String), which attempts
> > to substitute null values with the Connections host so there may be a bug
> > here.
> >
> > I'm happy to allow null returns here if necessary. Sean, can you
> > comment on the distinction between null & "" as far as hostname
> > verification is concerned?
> >
> > 2) In the latest iteration lookupEndpoints() returns an
> > Optional<LdapDnsResult>. Currently neither getEndpoints() nor
> > getDomainName() can be null. (they can be an empty list and/or an empty
> > String however)
> >
> > 3) Corrected.
> >
> > 4) See https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5
> >
> > -Rob
> >
>
