[EJBCLIENT-424] Refactor context data cleaning #607
Conversation
|
@chengfang @jbaesner could you please review? I think that after this fix we would only have jboss.returned.keys visible to the client (instead of all merged now) |
| if(returnedContextDataKeys != null) { | ||
| contextData.keySet().removeAll(returnedContextDataKeys); | ||
| } | ||
| contextData.keySet().removeIf(k -> (k != EJBClientInvocationContext.RETURNED_CONTEXT_DATA_KEY)); |
There was a problem hiding this comment.
This method needs clear doc stating what it does. In your latest change above, it now removes all entries from context data, except the one entry that specifies the jboss.returned.keys, and its value is a set of string keys. Is this what's intended here? This is not what the javadoc says.
Should we use equals instead of != for comparison?
There was a problem hiding this comment.
I have fixed the comparision.
What I have noticed during diagnosis of some test failure is that it currently works in the following way on the server side after the request is processed:
- the context has fields that the client put there and jboss.returned.keys
- later in the process (after the cleaning above) the server adds to the context those keys from jboss.returned.keys that were returned by the server
- as a result we have: client keys, "jboss.returned.keys", and those keys returned by the server that are present in jboss.returned.keys
I think that after this fix we would only have the latter two. this is not a bug but imho it will clean things a bit
Regarding the docs, the filling process happens after the cleaning so all the keys from jboss.returned.keys should be eventually present
tadamski
force-pushed
the
EJBCLIENT-424
branch
from
e574cf2 to
6f6a172
Compare
There was a problem hiding this comment.
I was recently working again on JBEAP-24454 which is related to this issue and I'm pretty certain that this fix is appropriate. I'm merging the PR.
| if(returnedContextDataKeys != null) { | ||
| contextData.keySet().removeAll(returnedContextDataKeys); | ||
| } | ||
| contextData.keySet().removeIf(k -> (k != EJBClientInvocationContext.RETURNED_CONTEXT_DATA_KEY)); |
There was a problem hiding this comment.
I have fixed the comparision.
What I have noticed during diagnosis of some test failure is that it currently works in the following way on the server side after the request is processed:
- the context has fields that the client put there and jboss.returned.keys
- later in the process (after the cleaning above) the server adds to the context those keys from jboss.returned.keys that were returned by the server
- as a result we have: client keys, "jboss.returned.keys", and those keys returned by the server that are present in jboss.returned.keys
I think that after this fix we would only have the latter two. this is not a bug but imho it will clean things a bit
Regarding the docs, the filling process happens after the cleaning so all the keys from jboss.returned.keys should be eventually present
No description provided.