Add support for idempotency keys when creating tickets#808
Add support for idempotency keys when creating tickets#808aleksei-averchenko-wise wants to merge 16 commits intocloudbees-oss:masterfrom
Conversation
PierreBtz
left a comment
There was a problem hiding this comment.
Hey! Thanks for the contribution. I did not have time yet to look into it in details, but I wonder if we could maybe improve the architecture.
What you suggest is to add the idempotency fields as part of the Ticket model, which seems to mix two different concerns:
- the ticket model itself
- the idempotency feature, which is not really part of the Ticket model but a feature of the ticket creation flow
As a consequence of this choice, it means that stored tickets/serialized ticket will keep those fields which have no real value after the ticket creation flow is done (if I have a null what does that mean exactly?).
A potential approach to this problem would be to avoid changing the Ticket model (keep it aligned with the ZD one) and create a TicketResult or even a ZendeskResult<T> class if we want to be future proof that would contain both the model and the additional feature of the creation flow, so roughtly:
public class CreateResult<T> {
private final T entity;
private final String idempotencyKey;
private final boolean idempotencyHit;
// getters setters...
// we really need to bump from java 11 to have records...
}
Then you would keep the current createTicket methods and create new methods:
public CreateResult<Ticket> createTicketIdempotent(Ticket ticket, String idempotencyKey) {
return complete(createTicketIdempotentAsync(ticket, idempotencyKey));
}
public ListenableFuture<CreateResult<Ticket>> createTicketIdempotentAsync(
Ticket ticket, String idempotencyKey) {
// create the request
// header addition
// completion handler specific to idempotent feature
// submit
}
and so on...
Call for end user would look like:
CreateResult<Ticket> result = zendesk.createTicketIdempotent(ticket, "key-123");
Ticket created = result.getEntity();
if (result.isIdempotencyHit()) {
// do stuff
}
Sounds good! I thought of that initially but then decided to be clever and try and sneak the feature into existing calls to make adoption easier on my end. I'll redo it the way you suggested. |
…A_HOME=$(/usr/libexec/java_home -v 11)` when running Maven to avoid compatibility issues.
|
@PierreBtz a couple of notes:
I tried to follow the contribution guidelines to the extent that the existing code follows them, hope that's good enough. Thanks! |
|
I updated |
|
I'm truly finished with |
PierreBtz
left a comment
There was a problem hiding this comment.
Sorry I didn't have time to completely review this this week. I'll do it next week.
I much prefer this iteration over the last one, thanks for doing the changes.
A couple of on going comments already though:
- I'm not exactly sure why the AI document mentions java 8 since the project now targets java 11, I'll need to double check if I missed something during the migration of the project or it's some hallucination. I'm pretty sure https://github.com/cloudbees-oss/zendesk-java-client/pull/808/changes#diff-a0ee9febc10ff56ffa42c0c33e8dbd38d5e3fbfe528574dfbe5c95e6f359a557R37 is a java 9 feature and wouldn't be 8 compatible.
- I still need to carefully review all the paths of the status check refactoring. At first glance it looks ok but I also have a feeling that we check the idempotency far more than we should.
For now I'll trigger the CI to see if the new tests pass against our sandbox.
|
@PierreBtz I think the Java 8 stuff in <checkSignatureRule implementation="org.codehaus.mojo.animal_sniffer.enforcer.CheckSignatureRule">
<signature>
<groupId>org.codehaus.mojo.signature</groupId>
<artifactId>java18</artifactId>
<version>1.0</version>
</signature>
</checkSignatureRule>I think what it does is it tries to validate the compiled bytecode against various JVM versions, as opposed to validating the Java source code itself. So even though we use lambdas left and right, it's okay because they get compiled away. I checked that VarHandle longArrayViewHandle = MethodHandles.byteArrayViewVarHandle(
long[].class, java.nio.ByteOrder.BIG_ENDIAN);(see https://en.wikipedia.org/wiki/Java_version_history#Java_SE_9 and https://openjdk.org/jeps/193). The compilation ( BTW I don't think there are any newer signature versions in the Maven repository (https://mvnrepository.com/artifact/org.codehaus.mojo.signature), and there is a deprectation warning in the log above. So it might be worth removing this rule if JVM 8 bytecode compatibility isn't intended. |
|
As for catching idempotency errors, my thinking was that
// org.asynchttpclient.DefaultAsyncHttpClient
private <T> ListenableFuture<T> execute(Request request, final AsyncHandler<T> asyncHandler) {
try {
return requestSender.sendRequest(request, asyncHandler, null);
} catch (Exception e) {
asyncHandler.onThrowable(e);
return new ListenableFuture.CompletedFailure<>(e);
}
} |
This change implements support for Zendesk's idempotency key
feature when creating tickets. I happen to need to use this feature myself :)
I added two new API methods:
createTicketIdempotentandcreateTicketIdempotentAsync. See the usage example in the updatedREADME.md.