The context :
since some days I try to find the root cause of a intermittent bug: all locations are returned ( with O2 and O3) during the login workflow for a user configured to see only one. It occurs randomly ( different users/test cases) during our automated tests and some end users noticed it too.
The investigation : with Ian, we found that the method Context.isAuthenticated can throw exception and Ian fixed it. I believe that Context.getAuthenticatedUser should be fixed as well because some ( a lot of) codes suppose that this method will return null and not that it will send an exception. This point can break the workflow in some DatafilterListener for instance.But even with these fixes the issue is still here.
I added a lot of debug messages to understand why the hibernate filters are not applied in some cases.The result is: DataFilterSessionContext supposes that we have a unique Hibernate session per thread. It uses ThreadLocal to determine if a session is configured or not. But we are not sure that the session instance is unique per thread and this fact is validated by my logs (we get a session with no filters for a non Daemon user whereas ThreadLocal variables state that it’s configured).
And ( didn’t check it) it could be possible that the method openSession is called by OpenMRS and maybe we should overwrite it too…( just a supposition here).
My question : why using ThreadLocal to store filters state for a Session and not put a property in the Hibernate session itself ? With this approach, we are sure the session is configured and we don’t have to clean the ThreadLocal variables ( by the way there are some cases like scheduled tasks for which the ThreadLocal are not cleaned at the end).
To sum up, I’m not sure that Thead Lifecycle is the same than Hibernate Session one.
Older versions of Hibernate did not have the setProperty function (this is actually part of the EntityManager interface and only became part of Session when Hibernate merged their EntityManager and Session types (I think that was Hibernate 5).
The lifecycle of OpenMRS Users is tied to the current thread, but you’re right that, clearly, threads are continuing to persist with this ThreadLocal even as the user changes (which is very bad).
What seems to be the underlying issue is that getAuthenticatedUser() throws an exception when called in a thread where a UserContext hasn’t been established. If this exception is thrown inside the getCurrentSession() function, data filter will halt adding new filters and mark the thread as having had filters applied, which is what is causing the issue. It’s often the case that background threads (particularly those driven by the events module or atomfeed) start without a UserContext (as a general principle OpenMRS expects each service to establish a user context either by spawning a DaemonThread to actually run changes or by authenticating as a user). I think it’s correct that Context throws an exception here, though maybe we could specialise the exception (e.g., to something like a UserContextNotInitializedException) and update the DataFilterSessionContext to deal with that appropriately.
We should probably have some consensus on how data filter should work in the absence of a valid user context.
Regardless of the mechanism used, it seems that marking either a thread or a Hibernate session as “Initialized” without an appropriate user context is an error. This is probably a case where it makes sense for DataFilterSessionContext to not mark either the session or the thread as initialized until it at least has a user.
It’s plausible for a thread to change user context after it has started, either by calling Context.authenticate() or Context.becomeUser() (though the later requires that the thread already be running as a superuser). I think that the implication of this is that we should not be just using a boolean to determine whether the session has been appropriately filtered or not, but something like the user_name or user_id, so that DataFilterSessionContext works appropriately when the thread’s user changes.