Skip to content
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

Allow JAX-RS endpoints Complete/Compensate to be invoked using any HTTP method #146

Closed
mmusgrov opened this issue Apr 29, 2019 · 26 comments · Fixed by #153
Closed

Allow JAX-RS endpoints Complete/Compensate to be invoked using any HTTP method #146

mmusgrov opened this issue Apr 29, 2019 · 26 comments · Fixed by #153
Assignees
Labels
PR sent The issue is waiting for its associated PR to be merged
Milestone

Comments

@mmusgrov
Copy link
Contributor

When the Compensate or Complete methods are JAX-RS endpoints they should be allowed to be annotated with any HTTP method, ie the following spec requirement is too restrictive:

The application developer indicates which JAX-RS method to use for a compensating
activity by annotating it with both the @Compensate and the JAX-RS @PUT annotations.

and

... If the annotation is not accompanied by
a JAX-RS @PUT annotation the error SHOULD be reported using a JAX-RS
exception mapper that maps to a 412 Precondition Failed HTTP status code.

@rdebusscher
Copy link
Member

ok for me.

@xstefank
Copy link
Member

+1, if no one else volunteers in between (I have already a few tasks at hand on the spec) I will do this when I'll get to it

@mmusgrov
Copy link
Contributor Author

Thanks @xstefank

@mmusgrov
Copy link
Contributor Author

Actually, I have just recalled why I mandated particular methods. It is because we need them for the REST based interop protocol. Can we say in the spec that participants SHOULD use the methods we already use (rather than MUST). That will make it easier for applications to work with the interop protocol we specify later.

@rdebusscher
Copy link
Member

@mmusgrov
We can always use the OPTIONS method and discover which http method is defined.

@mmusgrov
Copy link
Contributor Author

@rdebusscher Well spotted, thanks

@mmusgrov
Copy link
Contributor Author

It is standard REST practice to follow the guidelines in https://tools.ietf.org/html/rfc7231#section-8.1.3 for choosing which HTTP methods to use. It is also common when reporting a 202 Accepted status code to also include a link to a resource where the outcome of the request can be monitored.

With this comment in mind can we revert the changes made in the PR and go back to using MUST when defining which HTTP methods to use for participant JAX-RS endpoints.

@mmusgrov mmusgrov reopened this Jul 17, 2019
@tomjenkinson
Copy link
Contributor

I would support this, except in the circumstance that there would be some issue with fulfilling the requirement in an implementation in which case we should further evaluate.

@xstefank
Copy link
Member

I find this idea really useful because it will help with the specification adoption. Users will be allowed to just add LRA annotations to existing resource methods without the need to change them inside. But if we will be requiring PUT etc. again, users will need to change the API of existing resources (thus breaking clients) or create wrappers for existing methods which is not a good practice for REST.

However, other than usability, I don't have any other argument. So if there is an agreement we can revert this.

But this just covers going back to MUST for PUT, GET, DELETE. I am strongly against removing Status and requiring idempotency.

@tomjenkinson
Copy link
Contributor

I agree with Martins final sentiment, what we are talking about is the HTTP verbs and nothing else, correct?

@mmusgrov
Copy link
Contributor Author

The definition of idempotency matches our model: issuing the same resource update multiple times must have the same outcome. If the method is idempotent then performing

  PUT Compensated
    the participant state goes to Compensated

followed by

  PUT Compensated

then the only possible resulting state is Compensated (because it is idempotent).

On the other hand if the method is not idempotent and the request is resent then the resource would legitimately be allowed to go to some other state (such as Completed) because the method is not idempotent.

@mmusgrov
Copy link
Contributor Author

I agree with Martins final sentiment, what we are talking about is the HTTP verbs and nothing else, correct?

My first message talked about following the RFC guidelines which includes PUT being idempotent (and my last comment provided more justification). My message also said it is common when reporting a 202 Accepted status code to also include a link to a resource where the outcome of the request can be monitored. That is what I believe we are discussing.

The question about the usefulness of the status method if we choose to follow the IETF guidelines is a separate but related topic and can be discussed here or elsewhere.

@xstefank
Copy link
Member

The Status method takes the requirement of stating where the status information should be asked from away from the developer of the participant service and it is placing it on the LRA implementation. I understand your argument that 202 Accepted should be accompanied with the location of the status but this will be the responsibility of the end user who is developing business logic to remember every time when developing LRA service that 202 should have a location header. Why we can't ease the development of the LRA participant when we already have a mechanism for it with @Status? Maybe as a compromise can we support both @Status and location header for 202?

@tomjenkinson
Copy link
Contributor

Can we say something like if the participant supports PUT it must be idempotent and maybe when it registers with the LRA then it can notify it will be a PUT.

I am not sure if I saw somewhere on here confirmation that it is valid for PUT to return 202 as that might break idempotency? Or maybe because it is not definitive it is OK?

@tomjenkinson
Copy link
Contributor

@mmusgrov I am wondering about idempotency and how the participant is ever allowed to forget the state of the transactions.

PUT compensated -> 200 // participant will forget about the LRA now, is that not correct?
PUT compensated -> 404 (is that breaking idempotency or is it allowed?)

@mmusgrov
Copy link
Contributor Author

The Status method takes the requirement of stating where the status information should be asked from away from the developer of the participant service and it is placing it on the LRA implementation. I understand your argument that 202 Accepted should be accompanied with the location of the status but this will be the responsibility of the end user who is developing business logic to remember every time when developing LRA service that 202 should have a location header. Why we can't ease the development of the LRA participant when we already have a mechanism for it with @Status? Maybe as a compromise can we support both @Status and location header for 202?

I like this approach. So the implementation would autofill the Location Header if there is a method annotated with @Status. It should be reasonably easy to implement in the non JAX-RS case too.

@mmusgrov
Copy link
Contributor Author

Can we say something like if the participant supports PUT it must be idempotent and maybe when it registers with the LRA then it can notify it will be a PUT.

The proposal is that a JAX-RS compensation endpoint MUST use PUT and MUST follow the RFC guidelines (ie it has to be idempotent). Allowing different request methods is going to make things open to misinterpretation and will make implementations complicated since it will need to perform different logic depending on which request method the participant is using.

I am not sure if I saw somewhere on here confirmation that it is valid for PUT to return 202 as that might break idempotency? Or maybe because it is not definitive it is OK?

The RFC says:

A request method is considered "idempotent" if the intended effect on
   the server of multiple identical requests with that method is the
   same as the effect for a single such request.

So as long as the client is sending the same request he can expect the same effect on the server even if the server is still processing the first request.

@mmusgrov I am wondering about idempotency and how the participant is ever allowed to forget the state of the transactions.

PUT compensated -> 200 // participant will forget about the LRA now, is that not correct?
PUT compensated -> 404 (is that breaking idempotency or is it allowed?)

Section 4.4.2 says that the response may differ even though though duplicate requests have the same intended effect

...
It knows that repeating
   the request will have the same intended effect, even if the original
   request succeeded, though the response might differ.

@tomjenkinson
Copy link
Contributor

@mmusgrov so that looks like 202 -> 200 -> 404 is OK I suppose or even 202 -> 404 might be all that is visible to the client if responses are lost, still OK?

@mmusgrov mmusgrov self-assigned this Jul 22, 2019
@mmusgrov
Copy link
Contributor Author

A 202 response MUST include a URI in the location header where the caller can monitor progress (the implementation will "autofil" this header). So there should be no reason to call it again. But the caller MAY invoke the compensate end point again (since the proposal is that we follow the IETF guidelines for PUT which says it is idempotent) in which case you are correct: both 202 -> 200 -> 404 and 202 -> 404 would be valid sequences.

@mmusgrov
Copy link
Contributor Author

@rdebusscher You said on the other issue that including a link to a status URI in the compensate response was redundant because the implementation is calling the compensation.

This spec started out as a REST based protocol and was designed so that it could be used with non MicroProfile clients and participants (and in fact it is already being used in this way). I understand that MP is now the focus but if there is no good reason to step away from REST principles I would prefer to keep the spec as widely applicable as possible.

@tomjenkinson
Copy link
Contributor

Personally I feel that this as a new specification should be free to define the API in the most correct manner it can. I don't think that forcing existing applications to wrap endpoints with @put compensate/complete methods is unnecessarily burdonsome. I think unless there is some fundamental problem with @put for compensate/complete (as in it is not theoretically compatible with the purpose we want) then we should mandate it for correctness.

@rdebusscher
Copy link
Member

@rdebusscher You said on the other issue that including a link to a status URI in the compensate response was redundant because the implementation is calling the compensation.

And I still believe this and the JAX-RS approach will lead even to problems with consistency.

Having the following scenario

  • A @compensated @put annotated endpoint /compensate (participant)
  • A @Status @get annotated endpoint /status

When the end user calls the /compensate method with an active LRA id in the Header, the business logic will start the compensate actions for that Participant-LRA.
It receives a link to the status URI, and when calling this endpoint it receives Compensating, Compensated, or FailedToCompensate.
But at the implementation level, the LRA is still Active and so is the participant.
This is contradictory information.

And at the time the implementation calls the Compensate endpoint, the participant is allowed to response with a 404 (since compensation is completed)

So how should the implementation react on a 404 -> Cancelled or FailedToCancel?

Next remark isn't strictly related to this issue bit
The usage of 404 is very dangerous here (as already mentioned by Martin elsewhere) What if the participant is down and a 404 is returned? It should retry but that is a different case than the 404 due to the Participant already compensated by the end user.

But the implementation has no possibility to distinct the two.

So we clearly need more thoughts on the JAX-RS use cases.

@rdebusscher
Copy link
Member

When the 404 status code is not an issue (as suggested in issue 193) we still have the inconsistency to tackle when Compensate endpoint is called by the user and how the implementation should react on the 404 when it wants to cancel the LRA.

@mmusgrov
Copy link
Contributor Author

@rdebusscher You said on the other issue that including a link to a status URI in the compensate response was redundant because the implementation is calling the compensation.

And I still believe this and the JAX-RS approach will lead even to problems with consistency.

Having the following scenario

  • A @compensated @put annotated endpoint /compensate (participant)
  • A @Status @get annotated endpoint /status

When the end user calls the /compensate method with an active LRA id in the Header, the business logic will start the compensate actions for that Participant-LRA.
It receives a link to the status URI, and when calling this endpoint it receives Compensating, Compensated, or FailedToCompensate.
But at the implementation level, the LRA is still Active and so is the participant.
This is contradictory information.

And at the time the implementation calls the Compensate endpoint, the participant is allowed to response with a 404 (since compensation is completed)

So how should the implementation react on a 404 -> Cancelled or FailedToCancel?

Next remark isn't strictly related to this issue bit
The usage of 404 is very dangerous here (as already mentioned by Martin elsewhere) What if the participant is down and a 404 is returned? It should retry but that is a different case than the 404 due to the Participant already compensated by the end user.

But the implementation has no possibility to distinct the two.

So we clearly need more thoughts on the JAX-RS use cases.

Is your concern that unauthorised access to the compensate endpoint would confuse the spec implementation. We discussed this in issue #131 with the conclusion

that security is specifically not defined in the specification and that it is up to implementers to define the security elements of their approach

@mmusgrov
Copy link
Contributor Author

mmusgrov commented Jul 23, 2019

When the 404 status code is not an issue (as suggested in issue 193) we still have the inconsistency to tackle when Compensate endpoint is called by the user and how the implementation should react on the 404 when it wants to cancel the LRA.

Either the application would need to secure its endpoints (see #131) or we provide a security story in a later revision of the specification.

@tomjenkinson
Copy link
Contributor

+1 to securing the endpoints.

I guess the equivalent in a JTA space would be to telnet to the resource manager and issue commands to complete transaction branches, this would normally be prevented by security of some form (maybe password, or networking I suppose)

@mmusgrov mmusgrov removed the PR sent The issue is waiting for its associated PR to be merged label Jul 23, 2019
@mmusgrov mmusgrov added the PR sent The issue is waiting for its associated PR to be merged label Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR sent The issue is waiting for its associated PR to be merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants