eclipse / microprofile-lra

microprofile-lra
Apache License 2.0
101 stars 30 forks source link

SPEC: Make Type.MANDATORY the default and remove @LRA from class level #256

Closed rdebusscher closed 4 years ago

rdebusscher commented 4 years ago

Make Type.MANDATARY the default instead of Type.REQUIRED because;

When Type.MANDATORY is the default, using @LRA at class level will results in failures when there are JAX-RS defined callback methods (@Complete and @Compensate).

So ElementType.TYPE must be removed from @LRA.

xstefank commented 4 years ago

I don't see any big issue with this. If there is a requirement to keep class-level LRA, we must at least make it being ignored on methods annotated with different participant annotations (Complete, Compensate, Status, Leave, Forget) by default but I would also prefer to remove it from class-level for 1.0. For default type to be MANDATORY, your arguments make sense to me so +1. Only we will be not aligned with JTA's Transactional but that shouldn't be a big problem.

mmusgrov commented 4 years ago

How would we support the use case where the same logic should be reusable in the context of both an existing workflow and a new one. By way of illustration with your new suggestion:

client -> bookTrip (REQURES_NEW) -> bookFlight (MANDATORY) // works because there is a context

versus

client -> bookFlight (MANDATORY) // doesn't work because there is no context

Here the bookFlight business logic is not directly reusable. This makes it more difficult to compose resources into a workflow.

mmusgrov commented 4 years ago

I didn't understand what pitfalls around REQUIRED and the validation of the context was referring to since you would have the same issue with MANDATORY.

I also didn't understand the concern about using a class level LRA annotation. The callback methods are always invoked with a valid (though inactive) context so the implementation would see that the resource is already enlisted so it would effectively be a "null op". I looked at the spec around this situation and it wasn't clear so I think we can revisit what (we want) the spec to says about @LRA annotations on callback methods.

rdebusscher commented 4 years ago

Since a business flow has always a start and end step, I hope that developers are not going to use LRA as you propose here

You need a start action (LRA.Type.REQUIRES_NEW), the business actions ( LRA.Type.MANDATORY) and the end action ( LRA.Type.MANDATORY).

Those business actions can be reused in any flow between a start and end as they have LRA.Type.MANDATORY.

so LRA.Type.MANDATORY will be used in almost all case except the Start and thus this is the preferred default.

In your example, if developer wants to use it that way, he can specify Type.REQUIRED since it is a participant (since flight booking needs to be reversible) and thus there is no issue with validation.

rdebusscher commented 4 years ago

The difference of the default Type.REQUIRED and Type.MANDATORY is in that start action (which is a non-participant) of that business flow.

Developers will not bother specifying a type since everything seems to work correctly with the default (REQUIRED). But since the start action is not a participant, any value for the LRA Header will be allowed (since there is might be no validation at all in the current spec) And thus the business flow fails to correctly start an LRA in case the LRA header contains something.

When choosing MANDATORY as default, the developer is not required to specify any type except for the start event of the business flow. There he needs to specify REQUIRES_NEW. (and this will be clear during testing as it fails with the default)

rdebusscher commented 4 years ago

According to your interpretation of the spec, even MANDATORY doesn't trigger any validations (in the non-participant case). This means that URI:/this/will/be/accepted/as/LRA/header (literally that value) will be accepted as LRA header for the non-participant cases where no validation is performed. (including but not limited to the Types REQUIRED and MANDATORY)

That behaviour, anything is accepted, doesn't correspond with the name it has (MANDATORY)

tomjenkinson commented 4 years ago

According to your interpretation of the spec, even MANDATORY doesn't trigger any validations (in the non-participant case). This means that URI:/this/will/be/accepted/as/LRA/header (literally that value) will be accepted as LRA header for the non-participant cases where no validation is performed. (including but not limited to the Types REQUIRED and MANDATORY)

As long as the URI is a valid URI then my expectation would be that the LRA URI could reasonably be expected to be passed through until the point that some transactional action is attempted on the LRA referenced by the URI (such as enlistment of a full LRA participant or the registration of an AfterLRA). At that point the LRA will not be contactable (as presumably URI:/this/will/be/accepted/as/LRA/header will not resolve to an LRA - I suppose it could if it somehow was a valid resource) and the error would be expected to be reported then?

That behaviour, anything is accepted, doesn't correspond with the name it has (MANDATORY)

If someone has provided an invalid LRA header as the LRA ID then that would be an error, same as if they passing in "URI:foo:8080/some/other/". Perhaps "In case the implementation of this specification exposes non-JAX-RS participant methods to be able to call them externally (e.g. the HTTP proxy) then it MUST protect every exposed method from unauthorized access. The specific security details are not specified." could be expanded to make it clearer that is not specifically guarded against?

tomjenkinson commented 4 years ago

Specifically regarding changing the current default, could we define MP config settings for users/implementations to override the current default?

mmusgrov commented 4 years ago

Since a business flow has always a start and end step, I hope that developers are not going to use LRA as you propose here

With the current default (REQUIRED) if there is no incoming context then one is started automatically. That seems quite natural and the same logic can be reused regardless of whether there was a context present. If you now force services to explicitly demarcate the start of the flow (via REQUIRES_NEW) then you loose the ability to reuse methods (as my example tried to show) - with the new proposal the application would have to duplicate the business endpoint even if both endpoints implement the same behaviour. [Remark: I think this answer addresses your other points so I won't answer those separately).

tomjenkinson commented 4 years ago

My suggestion would be to keep the current default and provide a defined config method to override it. Would that not be acceptable?

If necessary I would think we should evaluate providing no default in 1.0 (maybe along with the config property?)

tomjenkinson commented 4 years ago

I was just chatting with @mmusgrov about this. I see now how a config property could be unsafe as it changes the developers original intention.

So I think we should evaluate providing no default in 1.0, which would leave it open to selecting a default 1.x+ then.

mmusgrov commented 4 years ago

My suggestion would be to keep the current default and provide a defined config method to override it. Would that not be acceptable?

If necessary I would think we should evaluate providing no default in 1.0 (maybe along with the config property?)

If we make default configurable then it would be more difficult for implementations to interoperate with each other and with themselves. Now the same app could be deployed to different servers/runtimes and at one server it behaves differently from another server depending upon how the config property is set.

It would also make it difficult for application developers to write code that was portable in different scenarios (since he now he has to write different code depending upon whether or not the config property has been set).

rdebusscher commented 4 years ago

Since we have decided to remove the non-participant scenario, this issue is probably less relevant.

Although developers will probably need much more type MANDATORY participants than REQUIRED, it will also work with with the latter since validation is performed (since everything needs to be a participant/party now).

If the rest of the group also wants to drop this issue, I can live with the fact that developers have to type a bit more.

xstefank commented 4 years ago

I agree that this is now less relevant especially in the 1.0 timeframe. Can we move this to 1.x and revisit when we have actual users' perspectives on this issue?

tomjenkinson commented 4 years ago

I think we should remove the default if we expect to revisit later.

mmusgrov commented 4 years ago

If the rest of the group also wants to drop this issue, I can live with the fact that developers have to type a bit more.

If there are no objects I will close this issue today?

rdebusscher commented 4 years ago

Closed as not important anymore.