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