You've cracked my elabourate webrev naming scheme. Thanks Daniel,
On 06/11/18 16:39, Daniel Fuchs wrote:
> Hi Rob,
>
> Assuming the new webrev is:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.09
>
> then it looks much better to me.
>
> I haven't re-reviewed everything in details - just looked
> at the updates (LdapDnsProviderService,
> LdapDnsProvider signature change, ServiceLocator)
>
Yep, apart from the volatile service nothing else has changed.
> BTW: if I'm not mistaken returning an empty Optional is
> equivalent, in the end, to returning an Optional containing
> a LdapDnsProviderResult which returns an empty list of endpoints?
> I guess that's OK, I don't see any value in preventing an
> LdapDnsProviderResult to have an empty endpoint list anyway.
>
Basically, yes.
Thanks,
-Rob
> best regards,
>
> -- daniel
>
>
> On 06/11/2018 15:52, Rob McKenna wrote:
> > 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
> > > >
> > >
>
