Add support for idempotency keys when creating tickets#808
Conversation
aleksei-averchenko-wise
requested review from
PierreBtz and
duemir
as code owners
PierreBtz
left a comment
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
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);
}
} |
| throw new ZendeskResponseRateLimitException(response); | ||
| } | ||
|
|
||
| if (isIdempotencyConflict(response)) { |
There was a problem hiding this comment.
There are two problems with this change:
- (minor): the json response is parsed for any non 2xx, 404 or 429 response. It adds some overhead. I think it's ok we can live with it, but still it's worth mentioning.
- (more problematic): if there is a parsing error for some reason, with the new code we will throw a
ZendeskResponseException(the actualJsonProcessingExceptionbeing the cause) while the previous code would have thrown aJsonProcessingException. This is a breaking change.
I see two options:
a. we accept the breaking change and document it in the release notes.
b. we add a flag in this method (like you already did with notFoundMeansNull to run the idempotency check) when relevant.
I would be leaning toward a, the new behavior makes sense, after all a wrong json makes sense as a ZendeskResponseException, folks can always unwrap if they want to know the cause. Thoughts?
There was a problem hiding this comment.
- It will only attempt parsing when
response.getStatusCode() != 400, so it doesn't seem like a huge issue to me since presumably we shouldn't be getting this error code for other reasons. - If I understand correctly, the code was previously never attempting to parse error response JSON, so the behaviour should be almost identical to how it was before. As an option, for extra clarity I can just swallow the exception in
isIdempotencyConflictso thatZendeskResponseExceptionis thrown exactly the way it was prior to this change by the last statement ofcheckStatusCode, with no suppressed exceptions added.
But like I said above, I'm open to adding another flag, it's that I would personally prefer to keep error handling as simple and unified as it can be.
There was a problem hiding this comment.
almost identical to how it was before
Indeed, there is a slight change (due to the fact that the handler now ends up parsing the json on non 2xx response, hence changing the exact exception error but not the exception type), my bad. I would not qualify this as breaking, if someone relies on the exact exception message in their logic, then I'm interested to see the bug report :)
| throw new ZendeskResponseRateLimitException(response); | ||
| } | ||
|
|
||
| if (isIdempotencyConflict(response)) { |
There was a problem hiding this comment.
almost identical to how it was before
Indeed, there is a slight change (due to the fact that the handler now ends up parsing the json on non 2xx response, hence changing the exact exception error but not the exception type), my bad. I would not qualify this as breaking, if someone relies on the exact exception message in their logic, then I'm interested to see the bug report :)
PierreBtz
left a comment
PierreBtz
left a comment
There was a problem hiding this comment.
Thanks for your contribution
|
My pleasure! |
2 participants
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.