search

eclipse / microprofile-context-propagation

Apache License 2.0
33 stars 23 forks source link

Make propagation of @ConversationScoped optional for MP CP impls #170

Closed manovotn closed 5 years ago

manovotn commented 5 years ago

Currently, the spec (with regards to CDI propagation) requires to support request/session/conversation context. It's not really in the /spec but I found a notion of that in ThreadContext javadoc. Link here.

There is also the CDI test in TCK suite, that is all-in-one for all scopes and requires conversation scope as well as the others.

I suggest to change this up and make @ConversationScoped support optional. The scope is pretty old and not so commonly used as the other two. In fact I would argue this is useful on bigger server side application (possibly clustered and all that) whereas in many other microprofile apps this doesn't really have a spot (request scope is by far the most used one). For instance in Quarkus (more accurately in Arc, it's CDI impl), there isn't even conversation scope at all and we have seen no requests for it so far. So, just like we support transaction propagation optionally, I think we should do the same for conversation scope.

Alternatively, we could change the TCK tests to first check if a context is active via BeanManager.getContext() and only test propagation if the given context is active and end the test (successfully) otherwise.

Hand in hand with that goes a split of the aforementioned TCK test to check each scope propagation separately. In fact I think we should do that either way. I'm fine sending PR for that, I'll just wait on some opinions here.

EDIT: corrected the link to the test, I meant CDIContextTest of course :)

So, WDYT?

njr-11 commented 5 years ago

Given that Quarkus/Arc doesn't have a conversation scope, I'd say you are already compliant. The correct way to propagate (or clear) and absent portion of context is to have the action/task run with that portion of context absent, which would be what you are already doing.

I think the right course of action is to update the TCK test to make it more granular and tolerant of first checking for availability before testing each scope as you have suggested.

kenfinnigan commented 5 years ago

If there's nothing in the spec dictating that specific scopes should be propagated, and it's more about whatever scopes exist get propagated, then I agree it makes sense to modify the TCK to check for a scopes presence before running the TCK for it

manovotn commented 5 years ago

Given that Quarkus/Arc doesn't have a conversation scope, I'd say you are already compliant.

Hmm, going point, you're right. Still, the TCK would need to be corrected to pass that. I'll send a PR for that tomorrow!