Hi Marek,
thank you for your quick response.
This is not only the problem in *userHandler.findUserByName()*. So I was
thinking about going with your 2nd solution because it's the root problem
and would fix problem more completely.
Thank you for your explanation again.
On 7 October 2014 14:08, Marek Posolda <mposolda(a)redhat.com> wrote:
Hi Trong,
On 7.10.2014 04:58, Trong Tran wrote:
Hi Marek and Boleslaw,
I see it was changing to use #flush and #handleException methods from
your commits
https://github.com/gatein/gatein-portal/commit/d21113ef7ce297b2f49e09fef8...
and
https://github.com/gatein/gatein-portal/commit/f90bcdddb76f0c52a39cab4cf6...
Do you have any idea on this problem ? we need your expertise :)
On 6 October 2014 23:33, Trong Tran <trong.tran(a)exoplatform.com> wrote:
> Hi,
>
> There is an issue related to IDM session transaction
>
https://issues.jboss.org/browse/GTNPORTAL-3539 which is potential I
> think.
>
> In summary of the problem, let's say we have following scenario:
>
> {code}
> // Some changes in organziation service, such as adding a new user *(1)*
>
> PicketLinkIDMOrganizationServiceImpl.flush();
> try {
> foundUser = session.getPersistenceManager().findUser(*userName*);
> } catch (Exception e) {
> handleException("Cannot obtain user: " + userName + "; ",
e); //
> rollback transaction
> }
>
> // The changes in (1) will be lost ==> *This is the current problem*
> {code}
>
> In the case of userName is NULL, the #findUser method will throws an
> exception AND inside the #handleException it will indirectly perform the
> rollback of IDM transaction (via #recoverFromIDMError) ==> losing changes
> from *(1)*
>
> There are two points that I really confuses in this scenario:
> 1. Does the PicketLinkIDMOrganizationServiceImpl.flush() perform saving
> the pending changes ? If NOT, I think there is a bug in IDM integration.
>
flush() synchronizes pending changes of your current transaction from
memory to database (PicketLinkIDMOrganizationServiceImpl.flush() is calling
hibernateSession().flush() under the hood). We found that this is good for
performance as with big number of users in DB, it might happen that
hibernate 1st level cache fills and it was very inefficient to call other
operations in same transaction without "flushing" in the meantime.
However flush doesn't call transaction commit. So if you have code like (I
am using pseudocode):
{code}
transaction.start()
createNewUser("joe");
orgService.flush();
// The user will be successfully returned here even if transaction is not
commited.
getUser("joe");
// transaction is rolled back. All DB modifications are lost
transaction.rollback();
// Starting new transaction here as previous one was rolled-back
transaction2.start();
// This will return null.
getUser("joe);
{code}
Hence it's expected that your unit test actually fails.
2. Should the rollback of IDM transaction be performed when any
> exception occurred in the #findUser method ?
>
IMO it's safer to perform rollback of transaction in case of unexpected
DB error. In this case it may not be the issue as the error is not related
to DB, but to unexpected argument passed to IDM. I thing that it would be
better to provide better (more fine-grained) exception handling, but on the
other catching just HibernateException wasn't always sufficient AFAIR...
For your issue I can see 2 solutions:
1) Avoid calling: userHandler.findUserByName(null);
Is it possible or is your integration layer done in a way, which doesn't
allow this?
2) If (1) is not possible, I would suggest to handle just specific
exception called by IDM PersistenceManagerImpl for null check. So I would
suggest something like:
try {
foundUser = session.getPersistenceManager().findUser(userName);
} catch (IllegalArgumentException iae) {
// findUser was called with null argument. Do something needed for
your business logic (Rethrow exception or at least log error)
} catch (Exception e) {
handleException("Cannot obtain user: " + userName + "; ", e);
}
This is workaround but should work. Is it ok with you?
Marek
> Could someone who has more expertise in IDM take a look and give any
> feedback/idea to help for handling this properly ?
>
> Thanks
>
> --
> Tran The Trong
> eXo Platform SEA
>
--
Tran The Trong
eXo Platform SEA