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/d21113ef7ce297b2f49e09fef819b3e96b86de06 and https://github.com/gatein/gatein-portal/commit/f90bcdddb76f0c52a39cab4cf61266b724259f92

Do you have any idea on this problem ? we need your expertise :)

On 6 October 2014 23:33, Trong Tran <trong.tran@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