eclipse / microprofile-lra

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

TckTests#nestedActivity() puts wrong expectations on the LRA_HTTP_CONTEXT_HEADER propagation #318

Closed xstefank closed 4 years ago

xstefank commented 4 years ago

The TckTest#nestedActivity() calls the LraResource#nestedActivity(URI, URI) which returns Response.ok(nestedLRAId).build() where the nestedLRAId is injected LRA_HTTP_CONTEXT_HEADER.

The test tries to also receive the content of the parent LRA id at line 141 with the following call:

Object parentId = response.getHeaders().getFirst(LRA_HTTP_CONTEXT_HEADER);

However, the response header called LRA_HTTP_CONTEXT_HEADER is not included in the response returned from the LraResouce endpoint.

As far as I am aware and by scanning the spec we are not specifying that the response headers should be implicitly filled for user calls and even if so this propagation doesn't make sense since the user is getting LRA_HTTP_CONTEXT_HEADER with the content of parentId so at least this should be LRA_HTTP_PARENT_CONTEXT_HEADER. I suggest to include the parent header in the response directly as we do in the other tests that need to read the LRA ids.

mmusgrov commented 4 years ago

This issue doesn't seem correct. I do not fully understand what problem is being reported but the test itself looks like what I intended it to do. I will go through it line by line explaining what the test does:

/**
 * 1. Start an LRA (line 127)
 * 2. Invoke a resource annotated with LRA.Type.NESTED (line 133)
 * (the resource method will run with a new nested context)
 * 3. When the resource method finishes the nested LRA will be closed
 * (because end is true: @LRA(value = LRA.Type.NESTED, end = true))
 * 
 * 4. The context that was active before the method was invoked should still be active
 * when the method returns and be present in the LRA context header (line 141)
 * returned to the client, there is an assert on line 144 that verifies that this is the case.
 * 5. Then verify that the response from the method contains the nested LRA (which will now be closed) (line 147)
 * 6. Verify that the nested LRA was closed (line 155)
 */

Do you object to my marking this issue with the invalid label?

xstefank commented 4 years ago

Do you object to my marking this issue with the invalid label?

Yes, I do, and next time please don't mark my work (issue/PR) as invalid before I have a chance to defend my position.

The issue is with your point 4.

* 4. The context that was active before the method was invoked should still be active
* when the method returns and be present in the LRA context header (line 141)

I took a deeper look at the spec and you are right, even response headers should propagate LRA_HTTP_CONTEXT_HEADER. However, I still don't understand why this should be the parentId in this case as the nestedLraId is still active when the response is returned. It is closed only after the JAX-RS methods ends. Can you explain this, please?

xstefank commented 4 years ago

The issue I've run to was indeed on the implementation side. I will close this issue but @mmusgrov please respond to my last question as I really don't understand how the propagation should behave according to the specification text.

mmusgrov commented 4 years ago

When the client issues the JAX-RS request the active context will be the one it created on line 127. When the invoked method begins a nested LRA is created (because of the LRA.Type.NESTED annotation element), therefore the active context will be this nested LRA. When the method finishes the LRA is closed (end = true) so when the method returns, the caller it should see the original context as the active one.

xstefank commented 4 years ago

So if it would be a top level LRA with end = true the response would not contain LRA_HTTP_CONTEXT_HEADER. Thanks.