Added config interceptor for in-memory server #1000
Conversation
Closes spring-projects#938 Signed-off-by: Emanuel Trandafir <[email protected]>
There was a problem hiding this comment.
Thanks, @etrandafir93! It's nice to have you contributing.
I've left some feedback inline. Also as a general note, will you please add JavaDoc to any new methods or classes that you create and in those JavaDoc ensure that you have the following:
@since 3.3| return newEmbeddedServer(defaultPartitionName, defaultPartitionSuffix, port, ConfigInterceptor.doNothing()); | ||
| } | ||
|
|
||
| public static EmbeddedLdapServer newEmbeddedServer(String defaultPartitionName, String defaultPartitionSuffix, |
There was a problem hiding this comment.
I'd prefer to not continue extending the list of parameters for this method. In fact, I think the existing static approach should likely be deprecated at some point.
Instead, let's create a Builder class that constructs an EmbeddedLdapServer. This will allow some reasonable defaults for the port (0) and partition name (the first dc) that the builder can take on, ultimately simplifying construction, e.g.:
Name suffix = LdapUtils.newLdapName("dc=jayway,dc=se");
EmbeddedLdapServer ldap = EmbeddedLdapServer
.withPartitionSuffix(suffix)
.config((config) -> config.setCodeLogDetails(...)).build();Separating this concern allows the instance to be started up more strategically.
There was a problem hiding this comment.
In going this route, will you please in a separate commit add a start method? It should wrap any UnboundID exceptions and rethrow a RuntimeException.
There was a problem hiding this comment.
Thanks for the feedback @jzheaux, I'll start working on these comments.
Since we're add the start() method, what do you think about implementing AutoClosable as well (and call the already existing shotdown() ) ?
For those who create the server programmatically using the builder, this will allow them to create it in a try-with-resources block.
There was a problem hiding this comment.
Yes, I think this is a good idea, @etrandafir93. Will you please open a ticket to add the start() and AutoCloseable and then reference that ticket in this separate commit?
test-support/src/main/java/org/springframework/ldap/test/unboundid/LdapTestUtils.java
Show resolved
Hide resolved
test-support/src/main/java/org/springframework/ldap/test/unboundid/EmbeddedLdapServer.java
Show resolved
Hide resolved
...ort/src/main/java/org/springframework/ldap/test/unboundid/EmbeddedLdapServerFactoryBean.java
Show resolved
Hide resolved
| @@ -97,8 +100,13 @@ public void setContextSource(ContextSource contextSource) { | |||
| this.contextSource = contextSource; | |||
| } | |||
|
|
|||
| public void setConfigInterceptor(ConfigInterceptor configInterceptor) { | |||
There was a problem hiding this comment.
Let's please leave this out of the TestContextSourceFactoryBean class for now. I'd prefer that folks configure the server and the LdifPopulator separately.
| @@ -225,3 +225,54 @@ To use it, create a bean similar to the following: | |||
| ==== | |||
|
|
|||
| Also, `org.springframework.ldap.test.unboundid.LdapTestUtils` provide methods to programmatically work with an embedded LDAP server. | |||
|
|
|||
There was a problem hiding this comment.
Can you move this to earlier in the doc and demonstrate it through the EmbeddedLdapServerFactoryBean class?
|
|
||
| [source,java] | ||
| ---- | ||
| LdapTestUtils.startEmbeddedServer(1888, "dc=jayway,dc=se", "jayway", |
There was a problem hiding this comment.
I think it would be better to focus on using EmbeddedLdapServer directly. Also, please consider wrapping this in a @Bean definition so that it's equivalent to the XML snippet.
|
Sorry about the DCO failure. I'll ping @rwinch and see if it is something on our side. |
|
@etrandafir93, I wonder if it is because the identifiers don't agree. On your commit, it shows the email as: But the Signed-Off-By is: |
|
Here is the error message when clicking the So I believe that may be the case. |
Closes #938