-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add a create/update/changeId/delete posthook (#193) #242
Conversation
@davidcoutadeur please rebase your PR since we have merged some important refactoring commits since you have sent this. |
Yes, I am working on this! Coming soon. |
f01fefd
to
59c91f6
Compare
Done |
The test is missing, I am working on it. |
59c91f6
to
6d51dfe
Compare
I have just added the test. Everything seems fine. |
I have some little remarks about the code itself, but nothing very important. Before that I would like to understand the purpose. I understand that you want to be able to launch a process after each modification/creation/deletion.
|
Hello
It is a topic open to discussion indeed. The main reason is that I'd like to keep the generic side of the LSC object
Good point indeed. I'll do the modification to use the standard input instead. |
LscModifications will always be the object to pass the modifications, whatever the protocol is used in the services. |
Sorry to step in, but we could have both, adding an option in LSC configuration to choose the input format for hooks ? |
Yes it could be an interesting option! Then did you check how binary attributes are encoded in JSON? |
I have pushed a new commit with:
|
I have made a check with jpegPhoto, which seems to be passed as base64 in LSC modification (and so in JSON) |
Also tested with The data seems encoded in base64 as well:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left several style comments, but this PR is pretty good!
I like the functionality and the implementation is pretty nice and straightforward, good job!
src/main/java/org/lsc/Hooks.java
Outdated
@@ -63,31 +64,56 @@ public class Hooks { | |||
* | |||
* return nothing | |||
*/ | |||
public final static void postSyncHook(final String hook, final LscModifications lm) { | |||
public final static void postSyncHook(final String hook, final String outputFormat, final LscModifications lm) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: in general in Java we don't use static methods. You can construct the object in the constructor of the caller.
src/main/java/org/lsc/Hooks.java
Outdated
String jsonModifications = null; | ||
|
||
String format = ""; | ||
if( outputFormat.equals("json") ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the parsing of the outputFormat should be done previously, here you should be able to manipulate only an Enum
src/main/java/org/lsc/Hooks.java
Outdated
if( format.equals("json") ) { | ||
modifications = getJsonModifications(lm); | ||
} | ||
else { | ||
modifications = LdifLayout.format(lm); | ||
} | ||
callHook("create", hook, lm.getMainIdentifier(), format, modifications); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these lines can be easily factorized, and in this case easily integrated in the callHook method
src/main/java/org/lsc/Hooks.java
Outdated
* Method calling the hook | ||
* | ||
* return nothing | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure such comments are really useful
@@ -197,6 +197,13 @@ public String getCondition(LscModificationType operation) { | |||
return result; | |||
} | |||
|
|||
public String getPostHookOutputFormat() { | |||
if (conf.getHooks() == null || conf.getHooks().getOutputFormat() == null) { | |||
return ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said earlier, the result of this method should be an enum.
Also, I would make clearer here which is the default value.
|
||
public String getCreatePostHook() { | ||
if (conf.getHooks() == null || conf.getHooks().getCreatePostHook() == null) { | ||
return ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: I usually prefer returning an Optional to indicate there is a value or not.
But here I did not check what are the consequences, it can be a little more complicated to handle if you are not used to.
reloadJndiConnections(); | ||
|
||
ObjectMapper mapperCreatedEntry = new ObjectMapper(); | ||
JsonNode expectedCreatedEntry = mapperCreatedEntry.readTree("[ { \"attributeName\" : \"objectClass\", \"values\" : [ \"inetOrgPerson\", \"person\", \"top\" ], \"operation\" : \"ADD_VALUES\" }, { \"attributeName\" : \"cn\", \"values\" : [ \"CN0001-hook\" ], \"operation\" : \"ADD_VALUES\" }, { \"attributeName\" : \"sn\", \"values\" : [ \"CN0001-hook\" ], \"operation\" : \"ADD_VALUES\" } ]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a binary attribute to your test? It will allow to check for regression regarding the handling of binary attributes in hooks.
hookReader.close(); | ||
hookFile.delete(); // remove hook log | ||
} catch (Exception e) { | ||
fail("Error while reading hook-ldif-" + operation + ".log"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't really need to try/catch in test, the test will fail anyway in case of unexpected exception.
hookReader.close(); | ||
hookFile.delete(); // remove hook log | ||
} catch (Exception e) { | ||
fail("Error while reading hook-json-" + operation + ".log"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
JsonNode hookOperation = mapper.readTree(jp); | ||
assertEquals(hookOperation, expectedEntry); | ||
} | ||
catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
Hello, I have taken all remarks in consideration, and made a new commit 68cd544 3 points remaining:
The naive way to add a custom binary attribute: (does not work because atob unavailability)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry again a bunch of comments, but it starts to look really nice!
src/main/java/org/lsc/Hooks.java
Outdated
|
||
if( hook != null && ! hook.equals("") ) | ||
if( hook != null && hook.isPresent() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks better.
The principle of an optional is to not be null, so you can remove the null check.
Also if instead of a String operationType you continue using LscModificationType, the code can be very simplified by:
hook.ifPresent(h -> callHook(lm.getOperation(), h, lm.getMainIdentifier(), outputFormat, lm)
src/main/java/org/lsc/Hooks.java
Outdated
LOGGER.error("Error while encoding LSC modifications to json"); | ||
LOGGER.error("encoding error: ", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOGGER.error("Error while encoding LSC modifications to json"); | |
LOGGER.error("encoding error: ", e); | |
LOGGER.error("Error while encoding LSC modifications to json", e); |
src/main/java/org/lsc/Hooks.java
Outdated
LOGGER.error("Error while calling {} posthook {} with format {} for {}", | ||
operationType, | ||
hook, | ||
outputFormat.toString(), | ||
identifier); | ||
LOGGER.error("posthook error: ", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOGGER.error("Error while calling {} posthook {} with format {} for {}", | |
operationType, | |
hook, | |
outputFormat.toString(), | |
identifier); | |
LOGGER.error("posthook error: ", e); | |
LOGGER.error("Error while calling {} posthook {} with format {} for {}", | |
operationType, | |
hook, | |
outputFormat.toString(), | |
identifier, | |
e); |
should work also
public String getCreatePostHook() { | ||
return ""; | ||
public Optional<String> getCreatePostHook() { | ||
return Optional.ofNullable(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Optional.ofNullable(null); | |
return Optional.empty(); |
public String getDeletePostHook() { | ||
return ""; | ||
public Optional<String> getDeletePostHook() { | ||
return Optional.ofNullable(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Optional.ofNullable(null); | |
return Optional.empty(); |
public String getPostHook(LscModificationType operation) { | ||
return ""; | ||
public Optional<String> getPostHook(LscModificationType operation) { | ||
return Optional.ofNullable(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Optional.ofNullable(null); | |
return Optional.empty(); |
public String getPostHook(LscModificationType operation) { | ||
String result = ""; | ||
public Optional<String> getPostHook(LscModificationType operation) { | ||
Optional<String> result = Optional.ofNullable(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can simplify this method by removing the result method and returning directly into each case
Hooks.postSyncHook(hook, outputFormat, lm); | ||
Optional<String> hook = syncOptions.getDeletePostHook(); | ||
OutputFormat outputFormat = syncOptions.getPostHookOutputFormat(); | ||
Hooks hookObject = new Hooks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hookObject (you can name it hooks) should be initialized in the constructor.
Hooks.postSyncHook(hook, outputFormat, lm); | ||
Optional<String> hook = task.getSyncOptions().getPostHook(modificationType); | ||
OutputFormat outputFormat = task.getSyncOptions().getPostHookOutputFormat(); | ||
Hooks hookObject = new Hooks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
Optional<String> hook = task.getSyncOptions().getPostHook(modificationType); | ||
OutputFormat outputFormat = task.getSyncOptions().getPostHookOutputFormat(); | ||
Hooks hookObject = new Hooks(); | ||
hookObject.postSyncHook(hook, outputFormat, lm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I think you can inline the parameters
commit 45d8a32 is fixing the last bunch of comments. |
No description provided.