[DISCUSS] Connector framework 1.4 enhancements

[Gael Allioux]

Hi,

here are the enhancements we [ForgeRock] would like to see in the next coming 1.4.X.Y release
of the connector framework.
Feel free to add/comment...

1- Improve Framework exceptions

We think about adding more exceptions to the framework.
SchemaViolationException for example

2- Allow distinction between CREATE and UPDATE in Sync operation

Currently, there is only DELETE or CREATE_UPDATE type of events in Sync.
Adding a CREATE and UPDATE distinctive events would be good.

3- Byte support

currently not supported as an attribute type

4-  __ANY__ special ObjectClass for Sync()

Sync() operation needs an objectClass to be passed to specify
which object class should be synced. In some situation, one may need
to get any kind of Sync events, regardless of the objectClass.
For that, we propose to create a special system __ANY__ objectclass that would
indicate the connector to collect any events.

5- Session Context in the pool

The poolable connectors do not have a way to persist connection information to optimize
connection handling. This can be workarounded using some static structures, but it is prone to
error and confusion.
The poolable connectors should be able to share connection informations between instances.

cheers
Gael

[Francesco Chicchiriccò]

On 02/07/2013 15:41, Gael Allioux wrote:

Hi,

here are the enhancements we [ForgeRock] would like to see in the next coming 1.4.X.Y release
of the connector framework.
Feel free to add/comment...

Hi Gael,
see my comments inline; one preliminary question, though: is there already any existing implementation / poc of the elements enlisted below?

Regards.

1- Improve Framework exceptions

We think about adding more exceptions to the framework.
SchemaViolationException for example

Nice idea in general: do you already have a complete set of exceptions to be added in mind?

2- Allow distinction between CREATE and UPDATE in Sync operation

Currently, there is only DELETE or CREATE_UPDATE type of events in Sync.
Adding a CREATE and UPDATE distinctive events would be good.

I am actually +-0 for this: could you provide some concrete use case where it makes sense to distinguish between CREATE and UPDATE during sync()?

3- Byte support

currently not supported as an attribute type

+1

4-  __ANY__ special ObjectClass for Sync()

Sync() operation needs an objectClass to be passed to specify
which object class should be synced. In some situation, one may need
to get any kind of Sync events, regardless of the objectClass.
For that, we propose to create a special system __ANY__ objectclass that would
indicate the connector to collect any events.

+1

5- Session Context in the pool

The poolable connectors do not have a way to persist connection information to optimize
connection handling. This can be workarounded using some static structures, but it is prone to
error and confusion.
The poolable connectors should be able to share connection informations between instances.

+1
Ideally implementing this would push some external dependency like as Commons Pool [1], but this would conflict with the "0 dependency" policy of the framework; from the other side, I am not that happy of reinventing the wheel for pool management...

[1] http://commons.apache.org/proper/commons-pool/

-- 
Francesco Chicchiriccò

ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/

[Fabio Martelli]

Il 03/07/2013 09:03, Francesco Chicchiriccò ha scritto:

On 02/07/2013 15:41, Gael Allioux wrote:

Hi,

here are the enhancements we [ForgeRock] would like to see in the next coming 1.4.X.Y release
of the connector framework.
Feel free to add/comment...

Hi Gael,
see my comments inline; one preliminary question, though: is there already any existing implementation / poc of the elements enlisted below?

Regards.

1- Improve Framework exceptions

We think about adding more exceptions to the framework.
SchemaViolationException for example

Nice idea in general: do you already have a complete set of exceptions to be added in mind?

+1

2- Allow distinction between CREATE and UPDATE in Sync operation

Currently, there is only DELETE or CREATE_UPDATE type of events in Sync.
Adding a CREATE and UPDATE distinctive events would be good.

I am actually +-0 for this: could you provide some concrete use case where it makes sense to distinguish between CREATE and UPDATE during sync()?

Maybe if you want to sync new entries only ...
+1 for me

3- Byte support

currently not supported as an attribute type

+1

+1

4-  __ANY__ special ObjectClass for Sync()

Sync() operation needs an objectClass to be passed to specify
which object class should be synced. In some situation, one may need
to get any kind of Sync events, regardless of the objectClass.
For that, we propose to create a special system __ANY__ objectclass that would
indicate the connector to collect any events.

+1

+1  .... but if this means that someone will be able t execute a generic search query for a generic object as well than ++1.
It would be nice to use a connector to execute a query out of the context; think about a query to ask for a particular [system] attribute value or similar.

5- Session Context in the pool

The poolable connectors do not have a way to persist connection information to optimize
connection handling. This can be workarounded using some static structures, but it is prone to
error and confusion.
The poolable connectors should be able to share connection informations between instances.

+1

+1

Ideally implementing this would push some external dependency like as Commons Pool [1], but this would conflict with the "0 dependency" policy of the framework; from the other side, I am not that happy of reinventing the wheel for pool management...

[1] http://commons.apache.org/proper/commons-pool/

"0 dependency"? Personally, I can't see why we should be compliant with this policy.

Best regards,
F.

-- 
Francesco Chicchiriccò

ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/

--
You received this message because you are subscribed to the Google Groups "connid-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to connid-dev+...@googlegroups.com.
Visit this group at http://groups.google.com/group/connid-dev.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[László Hordós]

Hi, 

I owe you with a more detailed explanation of these features and the reasons behind them. I wasn't prepared with that mail but I'm preparing it now and it will explain all these. Just to calm down a bit this thread. 

The CREATE and  UPDATE comes from https://wiki.evolveum.com/display/midPoint/ICF+Issues 

The pool requires more explanation but I'm not aware of how well you know the OpenICF Pool implementation. This feature we are talking about was implemented in OpenICF (so it's in ConnId now too) and does not add any new dependency. It's about some changes to the API and how to take benefit of what's there. 

Please let me explain most of these improvement  it with use cases later. I need to review the 1.4.0.0 branch and write this summary so I try to be quick.

 

Laszlo

 

[Radovan Semancik]

Hi,

On 07/03/2013 09:03 AM, Francesco Chicchiriccò wrote:

1- Improve Framework exceptions

We think about adding more exceptions to the framework.
SchemaViolationException for example

Nice idea in general: do you already have a complete set of exceptions to be added in mind?

I guess I can comment on this as this requirement originally comes from me. The basic idea is that any interface specification is incomplete unless also the set of error code (in this case exception classes) is specified. ICF interfaces usually do not specify any checked exceptions almost all exceptions are runtime. I do not like that approach but I can see that it is a new trend in Java programming so I can live with that. However, even though the exceptions are runtime the list of meaningful exceptions should really be specified, at least in the documentation. Otherwise the client code does not know what to check for. And if one wants to do error handling wee it ends up with a nightmarish code such as this:

https://svn.evolveum.com/midpoint/trunk/provisioning/provisioning-impl/src/main/java/com/evolveum/midpoint/provisioning/ucf/impl/IcfUtil.java

So, my complete proposal is: limit the number of exceptions that the ICF API and SPI can throw to a small meaningful set. And if not limit then at least strongly recommend. The set in org.identityconnectors.framework.common.exceptions seems to be quite good. However the primary problem is that a connector developer will easily overlook it. And as all the exceptions are runtime nothing really hints that there is a set of exceptions to use. It is not mentioned in the documentation or javadoc.  E.g. the javadoc for CreateApiOp.create specifies IAE, NPE and RuntimeException but none from the org.identityconnectors.framework.common.exceptions package. And even worse the CreateOp.create javadoc specfies no exceptions at all. This is what the connector developer sees. The developer may get an idea that throwing any arbitrary runtime exception is OK. Which is not.

Therefore the javadoc should be fixed first. ... and also any associated documentation ... if there is any.

When it comes to the list of exceptions in org.identityconnectors.framework.common.exceptions seems to be pretty much complete. Except for SchemaViolationException. I would really like to add that one.

FYI: there is another problem with the exceptions. As the connectors are running in their own classloaders the exceptions that are defined in a code loaded by the connector classloader is not really serializable. Well, it is rather pretty much serializable, but not deserializable. Deserialization fails because the upper classloader does not have access to the class loaded by the lower (connector) classloader. This is a problem e.g. for GUI that tries to store the exception in session to be used in complex GUI pages. However I do not know about any reasonable solution to this problem. Does anyone here knows?

-- 

                                           Radovan Semancik
                                          Software Architect
                                             evolveum.com

[László Hordós]

Hi,

I mainly agree with Radovan because we had this discussion long time ago. The exception usage in the connectors are very inconsistent due to the lack of the developers guide and I couldn't fix the contractor framework due to the same problem. Our goal now to write that documentation and align the contract test framework in regards. As I see ConnId is only interested in the Java Framework and Midpoint is in the local integration (This assumption is based on the IcfUtil.java) but OpenICF wants to keep the .Net and as well the remote connector server so the solution should be compatible and provide the same functionality. 

As Radovan mentioned the class loading problem. My opinion the ConnectorException should never wrap third-party classes and expose them to the application. These exceptions has to be "serialised" in some way and exposed as they would come from .Net or over the network.

The framework should only use the subclasses of ConnectorException and the subclasses of RuntimeException in Java5. You can ask why Java5, every application is tend to use the latest Java version and depends on them and this connector framework is the only last chance to make a bridge between a legacy IBM Java 5 API and any new applications. I'm not aware of any significant change in Java 6 or 7 we would require to depend on and drop the Java 5 support.

So my proposal is to collect somewhere on wiki our use cases and find the best solution. I think OpenIDM, MidPoint and Syncope has its own exceptions and adapter to adapt the ConnectorException exceptions to the application specific exceptions. I think this is where we can change the exception in a way it can produce meaningful messages and the application can use them better. These three product may have enough use-case coverage to verify the final design of the exceptions. I try to find the best match to map them to OpenIDM exceptions and if you can do the same with the other application then I think we get a very good result. 

                               

I try to summarise and start in this mail. This is what we have now. The ConnectorException simply extends RuntimeException and it has no added functionality/fields.

ConnectorException

AlreadyExistsException

ConfigurationException

ConnectorIOException

ConnectionBrokenException

ConnectionFailedException

ConnectorSecurityException.java

InvalidCredentialException.java

UnknownUidException.java

InvalidPasswordException.java

PasswordExpiredException.java

PermissionDeniedException

OperationTimeoutException.java        

I don't want to write the entire developers guide exception chapter here but I think we should start it somewhere on the wiki.

           

There must be some generic rules applicable for all operations.

    What exception to throw if the operation is not supported on the given objectClass? Mostly the application prevents to make such calls but the connectors must response in a common way.

  #1 Response with a ConnectorException and the message explains but we may want to throw some NotSupportedException from the application and then how to handle the catch?

#2 Response with a ConnectorException and extend the exception with some reason code. This keeps the number of exceptions low but we can go wild on the reason codes. 

#3 response with a new NotSupportedException class. 

What exception to throw if one of the required input parameter of the method is null? 

  I think this is handled by the Framework and we can guarantee it never happens. The framework throws NullPointerException or IllegalArgumentException and the Connector developer does not need to do anything.

All operation throws InvalidCredentialException if the remote framework key is invalid. This can be confused with the "authenticate" implementation exception unless ...

All operation throws PermissionDeniedException if the target resource will not allow to perform the operation. Very rare when the remote system throws such exception which allows to decide why the operation failed and throw this exception. I think this is just a nice to have exception. 

If the __UID__ is not known then thrown UnknownUidException seems a bit inconvenient unless it's known in what context it was thrown. For example the "authenticate" may turn into a Security exception and a CRUD operation may turn into a NotFoundException. 

What exception to throw if a single-valued attribute has more then one values.

I think this is where we need a SchemaViolationException with details, what's expected and what's the current value. 

I assume all three product use the TestOp if possible to validate the newly configured connector. That goes overt the initialisation process and then call the SPI operation.  We can take is as a generic scenario to call any SPI operation.

Configuration#validate

   This method throws very wide range of RuntimeException.

Connector#init

This method throws very wide range of RuntimeException.

  

PoolableConnector#checkAlive

This method throws very wide range of RuntimeException.

                 

TestOp 

This method throws very wide range of RuntimeException. 

These methods can throw any RuntimeException or Error or ClassNotFoundException etc.  

     

The CommonObjectHandlers lose some information because it serialises only the message and also the Exception class is change too unfortunately. I think this has to be fixed too.

The IllegalArgumentException and the ConnectorException kept as is but, all RuntimeException and sub-classes converted to new RuntimeException. This means the NullPointerException is converted too. All Throwable changed to new RuntimeException.

   

-----------------  

  

AuthenticateOp

If the username is not known then thrown UnknownUidException. 

If the password does not match then throw InvalidPasswordException.

If the password is expired then throw PasswordExpiredException.

What exception to throw if the account is disabled or locked? The InvalidCredentialException may get confused with the Remote "the remote framework key is invalid" case!!! 

            

Here I'd like to refer to package javax.security.auth.login and JSR-196 in regards the improvement.

AccountExpiredException Signals that a user account has expired.        MISSING

AccountLockedException Signals that an account was locked.             MISSING

AccountNotFoundException Signals that an account was not found. <-- UnknownUidException

CredentialExpiredException Signals that a Credential has expired.   <-- PasswordExpiredException

CredentialNotFoundException Signals that a credential was not found. MISSING 

FailedLoginException Signals that user authentication failed.   <-- InvalidPasswordException

  

ResolveUsernameOp

  If the username is not known then thrown UnknownUidException, otherwise follow the common guideline. 

CreateOp

What exception to throw if one of the Required attribute is empty/null (This is another question, what is the difference between, the attribute is not present, the value of attribute is null, the value of attribute is empty). 

I think this is where we need a SchemaViolationException with details, what's expected and what's the current value. 

What exception to throw if a not-creatable attribute was specified.

I think this is where we need a SchemaViolationException with details, explaining the value must be empty/null (see comment above!).

If the target object already exits then throw AlreadyExistsException.

The Connector implementer must make sure a failed operation is rolled back and it can be executed later. We know there are some system it's not possible. Once the object was created it can not be deleted any more and it can not be recreated. The connector itself is stateless so it has not any retry mechanism and it can not just try to update if the object is already exists because then the update and the create can not be differentiated and all operation would be an "upsert". The application can have its own retry implementation but it must know to change from create to update instead. To be able to signal this to the application we may need a PartiallyModifiedException or just a PartiallyCreatedException. This would mean the create operation must be changed to update and resend. The delete and update call should be resend.

 

DeleteOp

The Framework does not use any transaction management and I'd like to keep it as simple as we can but the optional MVCC revision has no costs. As I wrote before the Uid class was extended with revision property. This also requires some additional exception.

What exception to throw if the target resource requires the revision and it was not provided.  PreconditionRequiredException see:  428 Precondition Required

What exception to throw if the target resource requires the revision and it does not match.   PreconditionFailedException  see:  412 Precondition Failed

SchemaOp 

ScriptOnConnectorOp           

ScriptOnResourceOp    

SearchOp        

SyncOp           

         

UpdateAttributeValuesOp

There is one none clear use case but in case I mention it. An exception that is thrown during a operation on a resource when such an operation would result in a conflict. For example: when a patch/update conflicts with the object state. The object has the attribute with null value and the removeAttributeValues is called. see:  409 Conflict 

  

UpdateOp

What exception to throw if a not-updateable attribute was specified.

I think this is where we need a SchemaViolationException with details, explaining the value must be empty/null (see comment at CreateOp!).

I think this is enough now about the exception to initiate the discussion. Let me know what do you think before I go deeper. There are the other topics I need to explain in more details.

Laszlo

[Radovan Semancik]

Hi,

On 07/08/2013 12:40 PM, László Hordós wrote:

As Radovan mentioned the class loading problem. My opinion the ConnectorException should never wrap third-party classes and expose them to the application. These exceptions has to be "serialised" in some way and exposed as they would come from .Net or over the network.

Well, I initially thought so as well. But this will not work well. The ConnectorException subclasses has very little information about what went wrong. The original cause will be lost together with a stacktrace. This will make troubleshooting of the connector code very difficult. I've found that actually exposing inner exception through the API is a lesser evil. If they are exposed then the IDM may at least log a complete stacktrace and then get rid of it before passing that exception up. IDM can also inspect the inner exceptions for known problems of connectors that do poor exception handling. And let's not be naive. Doing proper error and exception handling is very difficult. Even though we fix the ICF exceptions and the documentation and tutorial there always be connectors that do poor error handling. If we disallow inner exceptions then what we will oftne get is just ConfigurationException("Boooo! Some parameter was wrong!"). And not even a line number to guess what parameter might be wrong. I guess that it will only make things worse.

So my proposal is to collect somewhere on wiki our use cases and find the best solution. I think OpenIDM, MidPoint and Syncope has its own exceptions and adapter to adapt the ConnectorException exceptions to the application specific exceptions. I think this is where we can change the exception in a way it can produce meaningful messages and the application can use them better. These three product may have enough use-case coverage to verify the final design of the exceptions. I try to find the best match to map them to OpenIDM exceptions and if you can do the same with the other application then I think we get a very good result.

We have already did this in midPoint. That's the reason I'm saying that I'm happy with the current set of exceptions (except for missing schema violation).

           

There must be some generic rules applicable for all operations.

Sure.

    What exception to throw if the operation is not supported on the given objectClass? Mostly the application prevents to make such calls but the connectors must response in a common way.

...

What exception to throw if one of the required input parameter of the method is null? 

  I think this is handled by the Framework and we can guarantee it never happens. The framework throws NullPointerException or IllegalArgumentException and the Connector developer does not need to do anything.

I don't think that we need to reinvent all the exceptions that are already in Java. I guess that throwing java.lang.UnsupportedOperationException is quite fine if an unsupported operation is invoked. Similarly IllegalArgumentException for API inputs that obviously violate interface contract (e.g. providing null where a non-null value is mandated). IlegalStateException may be used if an operation is invoked in wrong connector state (e.g. operation on uninitialized connector). I would propose that the framework should never throw NPE (unless there is a bug). IIRC the framework throws NPE now for some wrong inputs which is quite confusing. But if a connector throws NPE then I guess that might just get thrown from the API side as well without any change. The API consumer could (and really should) catch it and report in an appropriate way. But providing the original exception will help connector developer to diagnose the problem.

All operation throws InvalidCredentialException if the remote framework key is invalid. This can be confused with the "authenticate" implementation exception unless ...

Well, yes. When you mention this maybe we somehow need to distinguish internal ICF errors (e.g. error communicating with remote connector server) from the external errors (error communicating with the resource). I think that would be good thing to do.

If the __UID__ is not known then thrown UnknownUidException seems a bit inconvenient unless it's known in what context it was thrown. For example the "authenticate" may turn into a Security exception and a CRUD operation may turn into a NotFoundException.

I guess that well-written connectors usually have ways how to distinguish the situation when an object is not present (e.g. LDAP object not found) and when authentication failed due to other reasons (e.g. wrong password). Yes, I agree that there might be cases when this cannot be distinguished and in such a case it is OK to just throw the exception that is most appropriate. But I guess that UnknownUidException was created for operations such as update or delete where the object is explicitly addressed by UID.

What exception to throw if a single-valued attribute has more then one values.

I think this is where we need a SchemaViolationException with details, what's expected and what's the current value. 

Right, SchemaViolationException. But the details are tricky. Many systems will not provide any details or will provide the details only in unstructured text message. There also might be several attributes that violate the schema and may violate it in different way. And sometimes the situation is not that clear anyway (e.g. schema for "choice" attributes: one or the other needs to be present). Therefore if you want design it properly you will need quite a complicated structure to describe what caused the violation. We have tried the simple way in midPoint already. Our SchemaException has a field for attribute that caused the violation. But it is almost never used in practice. Well-specified text message usually describes the situation much better.

I assume all three product use the TestOp if possible to validate the newly configured connector. That goes overt the initialisation process and then call the SPI operation.  We can take is as a generic scenario to call any SPI operation.

Configuration#validate

   This method throws very wide range of RuntimeException.

Connector#init

This method throws very wide range of RuntimeException.

  

PoolableConnector#checkAlive

This method throws very wide range of RuntimeException.

                 

TestOp 

This method throws very wide range of RuntimeException. 

These methods can throw any RuntimeException or Error or ClassNotFoundException etc. 

I guess these also needs to be limited to the common set of exceptions. But I see that as a lower priority. The exceptions from TestOp are usually only displayed to the user. There is no compensation mechanism bound to them. So the computer-processability of there exceptions is not that important.

     

The CommonObjectHandlers lose some information because it serialises only the message and also the Exception class is change too unfortunately. I think this has to be fixed too.

I'm not sure what are you referring to? Are you referring to ResultHandler as used e.g. in SearchApiOp? Or some part of internal implementation? Is it something wrong with the interface contract or just implementation problem?

-----------------  

  

AuthenticateOp

If the username is not known then thrown UnknownUidException. 

If the password does not match then throw InvalidPasswordException.

If the password is expired then throw PasswordExpiredException.

What exception to throw if the account is disabled or locked? The InvalidCredentialException may get confused with the Remote "the remote framework key is invalid" case!!!

For disabled or locked I would use PermissionDeniedException. But I guess it is OK to use ConnectorSecurityException superclass for any case that is not properly described by its subclasses.

As for distinguishing framework problem from the resource problems as I mentioned above: yes would be a good idea. But how to do it elegantly without copy&pasting all the exceptions?

            

Here I'd like to refer to package javax.security.auth.login and JSR-196 in regards the improvement.

AccountExpiredException Signals that a user account has expired.        MISSING

AccountLockedException Signals that an account was locked.             MISSING

AccountNotFoundException Signals that an account was not found. <-- UnknownUidException

CredentialExpiredException Signals that a Credential has expired.   <-- PasswordExpiredException

CredentialNotFoundException Signals that a credential was not found. MISSING 

FailedLoginException Signals that user authentication failed.   <-- InvalidPasswordException

If you like to introduce such a fine-grained error codes I guess it will not harm. But honestly I doubt that they will be heavily used. AFAIK most resource interfaces will just indicate a generic "login failed" error anyway (due to "security reasons") so I guess that most connectors will not be able to distinguish these fine shades of grey. But again, it can't hurt and the set that is used by JSR-196 seems to be reasonable.

  

ResolveUsernameOp

  If the username is not known then thrown UnknownUidException, otherwise follow the common guideline.

Seems OK.

CreateOp

What exception to throw if one of the Required attribute is empty/null (This is another question, what is the difference between, the attribute is not present, the value of attribute is null, the value of attribute is empty). 

I think this is where we need a SchemaViolationException with details, what's expected and what's the current value.

Right.

What exception to throw if a not-creatable attribute was specified.

I think this is where we need a SchemaViolationException with details, explaining the value must be empty/null (see comment above!).

Yes.

If the target object already exits then throw AlreadyExistsException.

Yes.

The Connector implementer must make sure a failed operation is rolled back and it can be executed later. We know there are some system it's not possible.

I would say for most systems it is not possible in a generic case. Or at least not practical. I would not make this a hard requirement, but just a recommendation. Anyway, well-designed reconciliation process should recover any such half-finished operation.

Once the object was created it can not be deleted any more and it can not be recreated. The connector itself is stateless so it has not any retry mechanism and it can not just try to update if the object is already exists because then the update and the create can not be differentiated and all operation would be an "upsert". The application can have its own retry implementation but it must know to change from create to update instead. To be able to signal this to the application we may need a PartiallyModifiedException or just a PartiallyCreatedException. This would mean the create operation must be changed to update and resend. The delete and update call should be resend.

Introduce Partialy*Exception? Maybe. But there are cases when this cannot be detected anyway (e.g. timeout during waiting for create operation response). Therefore the API consumer cannot rely on this exception anyway and even ConnectorIOException may mean partially-executed operation. I think that practical value of such exception is very limited.

E.g. we have implemented error compensation and consistency system in midPoint almost a year ago. And we haven't needed such an exception. We rather need to know which exceptions are "safe" in terms that nothing was done and which are non-compensable (such as schema violation - that's why I miss it). All the rest must be considered as potential partial execution (including NPE, OOM, all the network errors, etc.). So I guess we do not need special "partial execution" exception. Just using any other than a "safe" exception is OK. And it may even better describe the error.

Surprisingly you will find that only a very little exceptions are really safe. E.g. if we mandate that "unknown uid" and "already exists" exceptions are consistency-safe then some connector operations will be difficult to implement. E.g. the infamous use of the secret ldapGroups attribute by LDAP connector. On the other hand if we mandate that "partial error" exception needs to be thrown in this case then then the original cause of the error may be lost. And the troubleshooting will become a nightmare. We may attempt to somehow merge these two concepts together but that may overcomplicate the system. And nobody will use it properly. This is very tricky. I would prefer simplicity and user-friendliness of the errors in this case. Anyway, the IDM needs to be very suspicious about what really happened on the resource in case of any error. I don't see that the framework can provide much assistance in this case.

 

DeleteOp

The Framework does not use any transaction management and I'd like to keep it as simple as we can but the optional MVCC revision has no costs. As I wrote before the Uid class was extended with revision property. This also requires some additional exception.

What exception to throw if the target resource requires the revision and it was not provided.  PreconditionRequiredException see:  428 Precondition Required

What exception to throw if the target resource requires the revision and it does not match.   PreconditionFailedException  see:  412 Precondition Failed

Right. Actually, the MVCC has some cost, at least in exceptions and testing. But I agree that the cost is low and that it well justified. The exceptions that you are proposing seem OK to me.

There is at least one more exception not to forget for DeleteOp: UnknownUidException

        

UpdateAttributeValuesOp

There is one none clear use case but in case I mention it. An exception that is thrown during a operation on a resource when such an operation would result in a conflict. For example: when a patch/update conflicts with the object state. The object has the attribute with null value and the removeAttributeValues is called. see:  409 Conflict 

  

UpdateOp

What exception to throw if a not-updateable attribute was specified.

I think this is where we need a SchemaViolationException with details, explaining the value must be empty/null (see comment at CreateOp!).

Important exception not to forget about for update-related operations is AlreadyExistsException. Update includes renames and even without rename update can easily hit unique constraints in the resource. This needs to be indicated somehow. Especially in rename cases.

[László Hordós]

Hi, 

I'm glad we are on the same page. Could you point me to your wiki where you have this developer guide? 

Hi,

On 07/08/2013 12:40 PM, László Hordós wrote:

As Radovan mentioned the class loading problem. My opinion the ConnectorException should never wrap third-party classes and expose them to the application. These exceptions has to be "serialised" in some way and exposed as they would come from .Net or over the network.

Well, I initially thought so as well. But this will not work well. The ConnectorException subclasses has very little information about what went wrong. The original cause will be lost together with a stacktrace. This will make troubleshooting of the connector code very difficult. I've found that actually exposing inner exception through the API is a lesser evil. If they are exposed then the IDM may at least log a complete stacktrace and then get rid of it before passing that exception up. IDM can also inspect the inner exceptions for known problems of connectors that do poor exception handling. And let's not be naive. Doing proper error and exception handling is very difficult. Even though we fix the ICF exceptions and the documentation and tutorial there always be connectors that do poor error handling. If we disallow inner exceptions then what we will oftne get is just ConfigurationException("Boooo! Some parameter was wrong!"). And not even a line number to guess what parameter might be wrong. I guess that it will only make things worse.

I agree with your all statements but I think we need to fix this problem anyway because it's not a good idea to fully rely on the cause and the third-party exception. I don't want to disallow it but I want to find a better alternative. It may be a nice addition and you can maintain your com/evolveum/midpoint/provisioning/ucf/impl/IcfUtil.java and glue each and every connectors. We need to find the solution when the exception comes from remote server and the stacktrace is lost then do something more then, ConfigurationException("Boooo! Some parameter was wrong!"). I don't know the solution yet because I'm now aware of all situation we must cover. This is why I want to write the developer guide first and fill the gaps. 

Just a wild idea after reading the entire mail I don't want to introduce error codes or so much new Exceptions for every case etc. and because we already wrap all Exception into a RuntimeException what about if we extend the CommonObjectHandlers to serialise the entire cause and on the server side it deserialize the stack down to the first ClassNotFound exception? I think it a good compromise. You can have you full stack trace as is now on local and from the remote server we can have the full JRE Throwable classes  and the .Net exceptions can be programatically mapped too.  

It may be a very complex implementation and requires continuous implementation change but it would not change the API and we can support more and more Exceptions just by changing the implementation and the Documentation and the Contract test can verify the ConnectorSecurityException wraps the AccountExpiredException if the the account is expired.

It would be very odd to catch the RuntimeException and check the cause :)

So my proposal is to collect somewhere on wiki our use cases and find the best solution. I think OpenIDM, MidPoint and Syncope has its own exceptions and adapter to adapt the ConnectorException exceptions to the application specific exceptions. I think this is where we can change the exception in a way it can produce meaningful messages and the application can use them better. These three product may have enough use-case coverage to verify the final design of the exceptions. I try to find the best match to map them to OpenIDM exceptions and if you can do the same with the other application then I think we get a very good result.

We have already did this in midPoint. That's the reason I'm saying that I'm happy with the current set of exceptions (except for missing schema violation).

           

There must be some generic rules applicable for all operations.

Sure.

    What exception to throw if the operation is not supported on the given objectClass? Mostly the application prevents to make such calls but the connectors must response in a common way.

...

What exception to throw if one of the required input parameter of the method is null? 

  I think this is handled by the Framework and we can guarantee it never happens. The framework throws NullPointerException or IllegalArgumentException and the Connector developer does not need to do anything.

I don't think that we need to reinvent all the exceptions that are already in Java. I guess that throwing java.lang.UnsupportedOperationException is quite fine if an unsupported operation is invoked. Similarly IllegalArgumentException for API inputs that obviously violate interface contract (e.g. providing null where a non-null value is mandated). IlegalStateException may be used if an operation is invoked in wrong connector state (e.g. operation on uninitialized connector). I would propose that the framework should never throw NPE (unless there is a bug). IIRC the framework throws NPE now for some wrong inputs which is quite confusing. But if a connector throws NPE then I guess that might just get thrown from the API side as well without any change. The API consumer could (and really should) catch it and report in an appropriate way. But providing the original exception will help connector developer to diagnose the problem.

I agree, I just want to document it in one place. You already answered few questions and I think you have the documentation of other situations as well. I didn't see much NPE from the framework but interesting proposal. I think we can keep eye on these NPEs and fix them if we see.

All operation throws InvalidCredentialException if the remote framework key is invalid. This can be confused with the "authenticate" implementation exception unless ...

Well, yes. When you mention this maybe we somehow need to distinguish internal ICF errors (e.g. error communicating with remote connector server) from the external errors (error communicating with the resource). I think that would be good thing to do.

I think this exception must be well differenciated from the operational exceptions. This is a broken connection within the Framework and it can be retried later or use the next available connector server, etc. 

I can think of these few cases but in these cases a cause can be examined:

• Connection failed because remote host is not accessible. 
• Connection failed because SSL configuration problem (javax.net.ssl.SSLHandshakeException, java.security.cert.CertificateException) 
• Connection failed because the remote framework key is invalid ----> (Change the implementation to not throw InvalidCredentialException)
• Connection failed because it was cut.
• Connection failed because timeout,

I need to investigate but I think in most of these cases the framework throw the original exception which is good for us.

If the __UID__ is not known then thrown UnknownUidException seems a bit inconvenient unless it's known in what context it was thrown. For example the "authenticate" may turn into a Security exception and a CRUD operation may turn into a NotFoundException.

I guess that well-written connectors usually have ways how to distinguish the situation when an object is not present (e.g. LDAP object not found) and when authentication failed due to other reasons (e.g. wrong password). Yes, I agree that there might be cases when this cannot be distinguished and in such a case it is OK to just throw the exception that is most appropriate. But I guess that UnknownUidException was created for operations such as update or delete where the object is explicitly addressed by UID.

I think I need to explain my concern better.

call authenticate(__ACCOUNT__, "non_existing", "password") throws UnknownUidException

call update(__ACCOUNT__, "non_existing", [changeAttributes]) throws UnknownUidException

I think I answered my question because in idm I need to convert the update:UnknownUidException to NotFound and the authenticate:UnknownUidException to AccountNotFoundException anyway this is a different issue I realised.

What exception to throw if a single-valued attribute has more then one values.

I think this is where we need a SchemaViolationException with details, what's expected and what's the current value. 

Right, SchemaViolationException. But the details are tricky. Many systems will not provide any details or will provide the details only in unstructured text message. There also might be several attributes that violate the schema and may violate it in different way. And sometimes the situation is not that clear anyway (e.g. schema for "choice" attributes: one or the other needs to be present). Therefore if you want design it properly you will need quite a complicated structure to describe what caused the violation. We have tried the simple way in midPoint already. Our SchemaException has a field for attribute that caused the violation. But it is almost never used in practice. Well-specified text message usually describes the situation much better.

I assume all three product use the TestOp if possible to validate the newly configured connector. That goes overt the initialisation process and then call the SPI operation.  We can take is as a generic scenario to call any SPI operation.

Configuration#validate

   This method throws very wide range of RuntimeException.

Connector#init

This method throws very wide range of RuntimeException.

  

PoolableConnector#checkAlive

This method throws very wide range of RuntimeException.

                 

TestOp 

This method throws very wide range of RuntimeException. 

These methods can throw any RuntimeException or Error or ClassNotFoundException etc. 

I guess these also needs to be limited to the common set of exceptions. But I see that as a lower priority. The exceptions from TestOp are usually only displayed to the user. There is no compensation mechanism bound to them. So the computer-processability of there exceptions is not that important.

     

The CommonObjectHandlers lose some information because it serialises only the message and also the Exception class is change too unfortunately. I think this has to be fixed too.

I'm not sure what are you referring to? Are you referring to ResultHandler as used e.g. in SearchApiOp? Or some part of internal implementation? Is it something wrong with the interface contract or just implementation problem?

If you don't use remote server then you never used this interface otherwise this limitation will hit you when you try to handle the exceptions. 

-----------------  

  

AuthenticateOp

If the username is not known then thrown UnknownUidException. 

If the password does not match then throw InvalidPasswordException.

If the password is expired then throw PasswordExpiredException.

What exception to throw if the account is disabled or locked? The InvalidCredentialException may get confused with the Remote "the remote framework key is invalid" case!!!

For disabled or locked I would use PermissionDeniedException. But I guess it is OK to use ConnectorSecurityException superclass for any case that is not properly described by its subclasses.

As for distinguishing framework problem from the resource problems as I mentioned above: yes would be a good idea. But how to do it elegantly without copy&pasting all the exceptions?

            

Here I'd like to refer to package javax.security.auth.login and JSR-196 in regards the improvement.

AccountExpiredException Signals that a user account has expired.        MISSING

AccountLockedException Signals that an account was locked.             MISSING

AccountNotFoundException Signals that an account was not found. <-- UnknownUidException

CredentialExpiredException Signals that a Credential has expired.   <-- PasswordExpiredException

CredentialNotFoundException Signals that a credential was not found. MISSING 

FailedLoginException Signals that user authentication failed.   <-- InvalidPasswordException

If you like to introduce such a fine-grained error codes I guess it will not harm. But honestly I doubt that they will be heavily used. AFAIK most resource interfaces will just indicate a generic "login failed" error anyway (due to "security reasons") so I guess that most connectors will not be able to distinguish these fine shades of grey. But again, it can't hurt and the set that is used by JSR-196 seems to be reasonable.

You right in most case not but there are many API which has this information in its AccessDenied exception and be honest, it's nice to have when you have a support case and you can say. "You must login first to AD and change the password, then you can use this authentication ;)" 

  

ResolveUsernameOp

  If the username is not known then thrown UnknownUidException, otherwise follow the common guideline.

Seems OK.

CreateOp

What exception to throw if one of the Required attribute is empty/null (This is another question, what is the difference between, the attribute is not present, the value of attribute is null, the value of attribute is empty). 

I think this is where we need a SchemaViolationException with details, what's expected and what's the current value.

Right.

What exception to throw if a not-creatable attribute was specified.

I think this is where we need a SchemaViolationException with details, explaining the value must be empty/null (see comment above!).

Yes.

If the target object already exits then throw AlreadyExistsException.

Yes.

The Connector implementer must make sure a failed operation is rolled back and it can be executed later. We know there are some system it's not possible.

I would say for most systems it is not possible in a generic case. Or at least not practical. I would not make this a hard requirement, but just a recommendation. Anyway, well-designed reconciliation process should recover any such half-finished operation.

Once the object was created it can not be deleted any more and it can not be recreated. The connector itself is stateless so it has not any retry mechanism and it can not just try to update if the object is already exists because then the update and the create can not be differentiated and all operation would be an "upsert". The application can have its own retry implementation but it must know to change from create to update instead. To be able to signal this to the application we may need a PartiallyModifiedException or just a PartiallyCreatedException. This would mean the create operation must be changed to update and resend. The delete and update call should be resend.

Introduce Partialy*Exception? Maybe. But there are cases when this cannot be detected anyway (e.g. timeout during waiting for create operation response). Therefore the API consumer cannot rely on this exception anyway and even ConnectorIOException may mean partially-executed operation. I think that practical value of such exception is very limited.

E.g. we have implemented error compensation and consistency system in midPoint almost a year ago. And we haven't needed such an exception. We rather need to know which exceptions are "safe" in terms that nothing was done and which are non-compensable (such as schema violation - that's why I miss it). All the rest must be considered as potential partial execution (including NPE, OOM, all the network errors, etc.). So I guess we do not need special "partial execution" exception. Just using any other than a "safe" exception is OK. And it may even better describe the error.

Surprisingly you will find that only a very little exceptions are really safe. E.g. if we mandate that "unknown uid" and "already exists" exceptions are consistency-safe then some connector operations will be difficult to implement. E.g. the infamous use of the secret ldapGroups attribute by LDAP connector. On the other hand if we mandate that "partial error" exception needs to be thrown in this case then then the original cause of the error may be lost. And the troubleshooting will become a nightmare. We may attempt to somehow merge these two concepts together but that may overcomplicate the system. And nobody will use it properly. This is very tricky. I would prefer simplicity and user-friendliness of the errors in this case. Anyway, the IDM needs to be very suspicious about what really happened on the resource in case of any error. I don't see that the framework can provide much assistance in this case.

Thanks, good to know. Then just forget it.

 

DeleteOp

The Framework does not use any transaction management and I'd like to keep it as simple as we can but the optional MVCC revision has no costs. As I wrote before the Uid class was extended with revision property. This also requires some additional exception.

What exception to throw if the target resource requires the revision and it was not provided.  PreconditionRequiredException see:  428 Precondition Required

What exception to throw if the target resource requires the revision and it does not match.   PreconditionFailedException  see:  412 Precondition Failed

Right. Actually, the MVCC has some cost, at least in exceptions and testing. But I agree that the cost is low and that it well justified. The exceptions that you are proposing seem OK to me.

There is at least one more exception not to forget for DeleteOp: UnknownUidException

        

UpdateAttributeValuesOp

There is one none clear use case but in case I mention it. An exception that is thrown during a operation on a resource when such an operation would result in a conflict. For example: when a patch/update conflicts with the object state. The object has the attribute with null value and the removeAttributeValues is called. see:  409 Conflict 

  

UpdateOp

What exception to throw if a not-updateable attribute was specified.

I think this is where we need a SchemaViolationException with details, explaining the value must be empty/null (see comment at CreateOp!).

Important exception not to forget about for update-related operations is AlreadyExistsException. Update includes renames and even without rename update can easily hit unique constraints in the resource. This needs to be indicated somehow. Especially in rename cases.

-- 

                                           Radovan Semancik
                                          Software Architect
                                             evolveum.com

[Francesco Chicchiriccò]

Lazlo, Radovan,
first of all, let me say that I agree in general with the content of this last e-mail.

May I ask you to (a) report and (b) continue discussing on ConnId wiki?
I have created [1] as general container and child [2] for exception management: I now need your user ids so that I can enable editing.

Regards.

[1] https://connid.atlassian.net/wiki/display/BASE/%5BDISCUSS%5D+ConnId+1.4.0.0
[2] https://connid.atlassian.net/wiki/display/BASE/Exception+Management

[László Hordós]

Hi

I'd like to write a bit more about the fifth improvement:

5- Session Context in the pool

To understand the use case you must know well the ObjectPool and the PoolableConnector relation. 

The PoolableConnector instances are stored in multiple pools and each pool identified by a unique ConnectorPoolKey which is composed by ConnectorKey,  ConfigurationProperties and ObjectPoolConfiguration. This works well when each PoolableConnector instances init and dispose a single reusable remote connection (for example a JDBC Connection). Each operation can borrow and existing PoolableConnector instance (Only one thread use this instance at that time, so the thread safety is guarantied!) from the pool or create a new one if the pool is empty and has available capacity. The PoolableConnector instances are disposed after certain idle time. This is the simple use case but most of the time the situation is more complex because  Some third-party libraries are involved.

The third-party Pool implementations and different SDKs have more advances requirements and some SDK offers only pooled connection. In such case there is no best practice to to implement connector. If a simple Connector implementation creates a new external pool then borrow one connection and finally dispose the entire pool for each operation is not optimal. If a PoolableConnector implementation creates a new external pool and borrow always one connection then this is not optimal either. 

The only workaround to share such "Object" (pool) is to create a static Map<"Key", "Object"> variable and get the external "Object" from there.  

-> First questions, what should be the "Key" value to avoid reusing the other "Object" made by other connector?

-> The OpenICF use the ConnectorPoolKey but it's not accessible. :( 

If there is a pool already then how easy would be to put the "Object" there so the Framework would guarantee that the PoolableConnector instances from the same pool can see the same  "Object".

This is where the Pool Context variable idea came from. This would allow to implement a PoolableConnector and this connector can init a third-party pool and share with the other instance from the same ObjectPool. Unfortunately this come with some additional complexity because now there are two kinds of pool are created and these has to be configured some way. For example  ObjectPool has max 10 and the third-party pool has max 8 means 2  PoolableConnector instances may not able to connect.

I think these has to be synchronised and it can only be done in the connector because the framework can not do this. This raise a risk of misconfiguring the pool from a broken PoolableConnector implementation.

Another use case when the configuration and the pool configuration should be in sync. The connector must connect to 3270 terminal and only one connection is allowed. This case the maxObjects must be set to 1. This is how the ObjectPool must be configured if the remote resource limits the max connections. Although the connector may use different connection which does not have this limit and the maxObjects can be more then 1.  In such case the ObjectPoolConfiguration may not in sync with ConfigurationProperties and the Connector may have the capability to validate/fix the ObjectPoolConfiguration in advance. This is how the Framework works now and no generic rule can validate this in the future either. This use case is just and addition to ObjectPoolConfiguration pre-syncronization/validation.

The last use case I have is the lifecycle of the third-party pool. When the first  PoolableConnector instance is created it has to initiate the third-party pool and when the last PoolableConnector instance is disposed it has to shutdown the third-party pool. 

#1 Before the ObjectPool is created the ObjectPoolConfiguration should be optimised by the PoolableConnector with regards the Configuration.

#2 Before the first PoolableConnector instance is used it has to initiates the PoolContext variables. 

#3 After the last PoolableConnector instance is disposed it also dispose the PoolContext variables.

In first round I'd like to validate/update these use cases in regards your experience with the Framework and in second round design the API to expose the required functionalities. The current code contains all the necessary implantation just the API has to be modified in a convenient way which I'd like to discuss in the second round.

Regards,

Laszlo  

[Francesco Chicchiriccò]

Hi Lazlo,
could you please use the wiki page [1] for this purpose?

Moreover, could you also report the current discussion's results about exception management into wiki page [2]?

Thanks!
Regards.

[1] https://connid.atlassian.net/wiki/display/BASE/Session+Context+in+the+pool
[2] https://connid.atlassian.net/wiki/display/BASE/Exception+Management

--
You received this message because you are subscribed to the Google Groups "connid-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to connid-dev+...@googlegroups.com.
Visit this group at http://groups.google.com/group/connid-dev.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Radovan Semancik]

On 07/11/2013 03:59 PM, László Hordós wrote:

Hi, 

I'm glad we are on the same page. Could you point me to your wiki where you have this developer guide?

We (Evolveum) don't have a developer's guide for connectors. Would be nice to have one, but we don't have it yet. Maybe a joint devel guide for ConnID/OpenICF would be a nice thing to have? I will gladly contribute.

On 07/08/2013 12:40 PM, László Hordós wrote:

As Radovan mentioned the class loading problem. My opinion the ConnectorException should never wrap third-party classes and expose them to the application. These exceptions has to be "serialised" in some way and exposed as they would come from .Net or over the network.

Well, I initially thought so as well. But this will not work well. The ConnectorException subclasses has very little information about what went wrong. The original cause will be lost together with a stacktrace. This will make troubleshooting of the connector code very difficult. I've found that actually exposing inner exception through the API is a lesser evil. If they are exposed then the IDM may at least log a complete stacktrace and then get rid of it before passing that exception up. IDM can also inspect the inner exceptions for known problems of connectors that do poor exception handling. And let's not be naive. Doing proper error and exception handling is very difficult. Even though we fix the ICF exceptions and the documentation and tutorial there always be connectors that do poor error handling. If we disallow inner exceptions then what we will oftne get is just ConfigurationException("Boooo! Some parameter was wrong!"). And not even a line number to guess what parameter might be wrong. I guess that it will only make things worse.

I agree with your all statements but I think we need to fix this problem anyway because it's not a good idea to fully rely on the cause and the third-party exception. I don't want to disallow it but I want to find a better alternative. It may be a nice addition and you can maintain your com/evolveum/midpoint/provisioning/ucf/impl/IcfUtil.java and glue each and every connectors. We need to find the solution when the exception comes from remote server and the stacktrace is lost then do something more then, ConfigurationException("Boooo! Some parameter was wrong!"). I don't know the solution yet because I'm now aware of all situation we must cover. This is why I want to write the developer guide first and fill the gaps.

I don't know the solution either. That's why I was asking about it :-) ... so we probably need to continue looking for a solution. In the meantime we perhaps need:
1) Document the correct use of exceptions by the connectors (javadoc, devel guides)
2) Fix the use of exceptions in connectors that are under our control (in natural development pace)
3) Discourage use of inner exceptions if a standard ICF exception is used with a good error message.
4) ... but still allow the use of inner exceptions ...
5) ... and expect "ugly" exceptions and keep the nightmarish if-then-else exception parsing code as a last resort.
6) Continually look for any better solution

Just a wild idea after reading the entire mail I don't want to introduce error codes or so much new Exceptions for every case etc. and because we already wrap all Exception into a RuntimeException what about if we extend the CommonObjectHandlers to serialise the entire cause and on the server side it deserialize the stack down to the first ClassNotFound exception? I think it a good compromise. You can have you full stack trace as is now on local and from the remote server we can have the full JRE Throwable classes  and the .Net exceptions can be programatically mapped too.

I'm not sure I understand your idea. Are you trying to fix the problem of exception passing from remote connector server to the framework? If that is the case then I don't think serialization will work, e.g. in case of .net server and java framework. And actually there may be more server implementations in the future not just java and .net. So we probably need a proper platform-independent solution here. And I guess the simples one can also be the best (at least for now): just dump the exception with the stacktrace to string at the server side and pass it as a string "detailed error report" to the framework.

Of course there also needs to be .net representation of all the standard ICF exceptions and these should be mapped one-to-one by the framework. But these may also contain the string dump of the primary cause - which may be essential to diagnose a code of a remote connector. Local logging of a remote connector server might be used instead ... but that's complicated. Firstly the access to a remote connector server machine is usually problematic in practice, especially in heretogeneous environment where the remote connector server is usually used. Secondly, I would like to see a "zero maintenance" mode of remote connector server setup. Which means no logfiles on remote side. Which means sending all diagnostic data to the framework (think about OpenAM policy agents). And thirdly, correlating IDM logfile with a remote connector server logfile is real pain. Cause-and-effect is very difficult to follow especially on a busy system. Therefore I believe that it is essential that remote connector server returns as much diagnostic data as practically possible in an error response.

Or is it something else that you are referring to?

Well, yes. When you mention this maybe we somehow need to distinguish internal ICF errors (e.g. error communicating with remote connector server) from the external errors (error communicating with the resource). I think that would be good thing to do.

I think this exception must be well differenciated from the operational exceptions. This is a broken connection within the Framework and it can be retried later or use the next available connector server, etc. 

I can think of these few cases but in these cases a cause can be examined:

• Connection failed because remote host is not accessible. 
• Connection failed because SSL configuration problem (javax.net.ssl.SSLHandshakeException, java.security.cert.CertificateException) 
• Connection failed because the remote framework key is invalid ----> (Change the implementation to not throw InvalidCredentialException)
• Connection failed because it was cut.
• Connection failed because timeout,

I need to investigate but I think in most of these cases the framework throw the original exception which is good for us.

Hmmm. Maybe I don't understand you again. Let's imagine that the client code (openidm/syncope/midpoint) caught a ConnectionFailedException. How can the code determine whether the source is a connector or a framework? I guess that both of these might use java.net.SocketTimeoutException as a cause. And even if there is a hostname/port somewhere in the message the code will not look into a message to distinguish the cases. And that can even be confusing to a human operator if the connector server and the resource reside on the same host (which is common). Maybe if could be distinguished by looking at the stacktrace but that seems quite crazy to me.

I guess what we need is some kind of "error origin" definition directly in the exceptions (as fields). E.g. specification of connector instance and remote connector server instance which caused the error.

If the __UID__ is not known then thrown UnknownUidException seems a bit inconvenient unless it's known in what context it was thrown. For example the "authenticate" may turn into a Security exception and a CRUD operation may turn into a NotFoundException.

I guess that well-written connectors usually have ways how to distinguish the situation when an object is not present (e.g. LDAP object not found) and when authentication failed due to other reasons (e.g. wrong password). Yes, I agree that there might be cases when this cannot be distinguished and in such a case it is OK to just throw the exception that is most appropriate. But I guess that UnknownUidException was created for operations such as update or delete where the object is explicitly addressed by UID.

I think I need to explain my concern better.

call authenticate(__ACCOUNT__, "non_existing", "password") throws UnknownUidException

call update(__ACCOUNT__, "non_existing", [changeAttributes]) throws UnknownUidException

I think I answered my question because in idm I need to convert the update:UnknownUidException to NotFound and the authenticate:UnknownUidException to AccountNotFoundException anyway this is a different issue I realised.

You can convert it quite easily because you know which method you have invoked, right? Or am I missing something?

           

Here I'd like to refer to package javax.security.auth.login and JSR-196 in regards the improvement.

AccountExpiredException Signals that a user account has expired.        MISSING

AccountLockedException Signals that an account was locked.             MISSING

AccountNotFoundException Signals that an account was not found. <-- UnknownUidException

CredentialExpiredException Signals that a Credential has expired.   <-- PasswordExpiredException

CredentialNotFoundException Signals that a credential was not found. MISSING 

FailedLoginException Signals that user authentication failed.   <-- InvalidPasswordException

If you like to introduce such a fine-grained error codes I guess it will not harm. But honestly I doubt that they will be heavily used. AFAIK most resource interfaces will just indicate a generic "login failed" error anyway (due to "security reasons") so I guess that most connectors will not be able to distinguish these fine shades of grey. But again, it can't hurt and the set that is used by JSR-196 seems to be reasonable.

You right in most case not but there are many API which has this information in its AccessDenied exception and be honest, it's nice to have when you have a support case and you can say. "You must login first to AD and change the password, then you can use this authentication ;)"

OK, no problem. We can add these exceptions.

[Radovan Semancik]

Hi Francesco,

I don't think it is a good idea to discuss things at two places at one, especially if one of them is not really good for discussion (wiki). Let's continue discussion here and just document the consensus in wiki (if really necessary). Or even better document it in the form of resulting source code. All the important decisions (and even part of the motivation) needs to go to the javadoc anyway. This is part of the interface contract. Therefore if we do not have consensus yet let's continue discussion here. If we have the consensus let's update the source code right away (e.g. in a topic branch if we are not yet sure about it).

Let's keep the unnecessary bureaucracy to the minimum, please.

-- 

                                           Radovan Semancik
                                          Software Architect
                                             evolveum.com

On 07/12/2013 11:45 AM, Francesco Chicchiriccò wrote:

[Francesco Chicchiriccò]

On 15/07/2013 11:14, Radovan Semancik wrote:

Hi Francesco,

I don't think it is a good idea to discuss things at two places at one, especially if one of them is not really good for discussion (wiki). Let's continue discussion here and just document the consensus in wiki (if really necessary). Or even better document it in the form of resulting source code. All the important decisions (and even part of the motivation) needs to go to the javadoc anyway. This is part of the interface contract. Therefore if we do not have consensus yet let's continue discussion here. If we have the consensus let's update the source code right away (e.g. in a topic branch if we are not yet sure about it).

Let's keep the unnecessary bureaucracy to the minimum, please.

+1
but I don't think it is "unnecessary bureaucracy", I was just remembering to use *only* the wiki for discussion, as we decided at the beginning.

If you are more comfortable in this way, let's continue if others (mainly Lazlo) agrees.

Regards.

On 07/12/2013 11:45 AM, Francesco Chicchiriccò wrote:

Lazlo, Radovan,
first of all, let me say that I agree in general with the content of this last e-mail.

May I ask you to (a) report and (b) continue discussing on ConnId wiki?
I have created [1] as general container and child [2] for exception management: I now need your user ids so that I can enable editing.

Regards.

[1] https://connid.atlassian.net/wiki/display/BASE/%5BDISCUSS%5D+ConnId+1.4.0.0
[2] https://connid.atlassian.net/wiki/display/BASE/Exception+Management

[László Hordós]

Hi,

I agree with Radovan. Keep the discussion here and put the consensus in wiki or source code.

Laszlo

[Francesco Chicchiriccò]

On 15/07/2013 11:44, László Hordós wrote:

Hi,

I agree with Radovan. Keep the discussion here and put the consensus in wiki or source code.

Fine, then, let's keep the discussion here.

Regards.

[László Hordós]

Hi

I'd like to write a bit more about the fourth improvement:

4-  __ANY__ special ObjectClass for Sync()

This is a special case when the order of sync events is important across multiple ObjectClasses. Most of SPIOperation is specific for a given ObjectClass but there are two exceptions when IDM needs to retrieve different ConnectorObjects with different ObjectClasses.

Sync: If there is a relation between object type in the remote resource, for example  "Container" contains "Object" and "Object" has additional "Sub-Objects" and the change events has to be processed in exact same order as it ordered on the resource then the Sync must be able to request the all events for all the three ObjectClasses in one operation.

Search: This has the same limitation, the Connector can not support a Polymorphic relation. For example to list the member of a Group if the members can be Group or User. 

In these cases the Framework would allow to call sync(__ANY__, … ) and executeQuery(__ANY__, ..) but it would throw IllegalStateException or UnsupportedOperationException in any other case. 

Regards,

Laszlo

[László Hordós]

Hi,

Radovan pointed out a very important problem with the origin of the Exception. 

The full call chain may have two I/O channels and if both are network connection then very likely they throw the same exception and IDM can not distingush between them.  Q: Does it really need to? 

[IDM] --I/O--> [ConnectorServer] --I/O--> [Resource] 

We have full control over the network connection between the IDM and ConnectorServer and we can catch any Throwable and for example wrap them into ConnectorIOException and label them as "Come from  ConnectorServer IO" Exception. We can expect any kind of exception from the second I/O between the Connector and the Resource. I think it does not really matters unless there is another ConnectorServer and we can fail over to that. In that case the exception comes from ConnectorServer and then it matters and it answers the first question. 

What really matters is where in the call stack the exceptions comes from. The API operation call goes through these steps and any of them may throw exception and we can not identify the source.

• validate
• init
• checkAlive
• operation
• dispose

Q: Why is it important?

If validate throws IllegalArgumentException then the configuration is wrong and it's a permanent exception, if the create operation throws IllegalArgumentException then the request contains something wrong. Both may be handled differently in IDM.

Validation Failed -> Permanent Exception because the configuration is invalid.

Init or CheckAlive Failed -> Retryable Exception because the remote server may be down at the moment.

Operation Failed -> Permanent Exception because the bad request/attributes was supplied or Permission is denied or Operation is not supported.

Dispose Failed -> Diagnostic report to IDM because the allocated resources has to be discharged at some point.

All these can thrown any Connector I/O Exception which makes it Retryable exception.

We can force IDM to call the validate and be sure when we call the operation (for example create) the validate won't fail. Still there is init, checkAlive and after a success operation the dispose and all these may be over any I/O connection. 

Connect to remote server [OK/Fail] --> validate[OK/Fail]

Connect to remote server [OK/Fail] --> validate[OK]  --> init[OK/Fail] --> checkAlive[OK/Fail] --> operation[OK/Fail] --> dispose [OK/Fail]

Q: What if Validate throws NullPointerException/IllegalArgumentException or the operation throws it? I think this is why it's important.

@Radovan: I remember once you wrote something about NullPointerException/IllegalArgumentException and why or where (do not) use NullPointerException but I didn't find it.

Conclusion:

If the Connector can throw any exception then the Framework must catch it and:

A: Wrap it into an exception and mark it as PRE_OPERATION, OPERATION, [POST_OPERATION] exception and throw it up to IDM.

B: Change the API to use a CallBackHandler and framework can call the appropriate method with the exception. 

I prefer the B option because that is closer the to the solution I would like to propose for the query implementation where the Framework implementaion has a synchronous API then asynchronous implementation and then the API is a mix of sync/async. I will explain it in another mail later where I collect the issues with the query. This mail is long enough. 

There is an example ResultHandler.java in advance.

I noticed it's very hard to collect all cases and write a developers guide but as Radovan mentioned its place should be the JavaDoc so I will implement a reference in memory Connector and update the JavaDoc and the ConnId wiki page. I think that's the best to be able see all cases and If there is a grey area I'm going to send a mail. I use this connector to test the Contract Framework so it's almost done.

Just an example of what exceptions should/can be thrown:

CreateApiOp:

Framework throw:

IllegalArgumentException - if ObjectClass is missing or elements of the set produce duplicate values of Attribute.getName().

NullPointerException - if the parameter createAttributes is null.

ConnectorIOException, ConnectionBrokenException, ConnectionFailedException - if any problem occurs with the connector server connection.

CreateOp:

Connector throw:

UnsupportedOperationException - if the Create operation on given ObjectClass is not supported.

SchemaViolationException - if required attribute is missing, not createable attribute is present or attribute has invalid value.

AlreadyExistsException - If the object with the given Name already exits.

PermissionDeniedException - If  the target resource will not allow a Connector to perform a particular operation.

ConnectorIOException, ConnectionBrokenException, ConnectionFailedException - if any problem occurs with the connection.

RuntimeException - If anything goes wrong. Try to throw native exception 

I noticed nobody knows well how the exceptions are handled if they are serialised and received from a remote server. For the better understanding the problem and the solution I wrote this sample project and  I documented so you can play with it and see What exception is received in IDM if a remote connector throws a NumberFormatException and what happens with the original cause and the stack trace. I think if you see it as a Java code it tells you more then I can explain the problem.

From the log you can see if a remote java connector throws AssertionError then IDM receives RuntimeException.

   Call [facade ]:

Throw [Error   ]

Catch [remote]: class=AssertionError message=Missing attribute cause=null

Catch [IDM     ]: class=RuntimeException message=Missing attribute cause=null

From the log you can see if a remote .Net connector throws Microsoft.Build.BuildEngine.InvalidToolsetDefinitionException then IDM receives RuntimeException.

    Call [facade]:

Throw [UnknownException]

Catch [dotnet]: class=Microsoft.Build.BuildEngine.InvalidToolsetDefinitionException message=Example Message cause=null

Catch [IDM   ]: class=RuntimeException message=Microsoft.Build.BuildEngine.InvalidToolsetDefinitionException cause=null

We need to bring together the .Net System.Exception and the Java Error/Exception/RuntimeException management and the the way they are serialised in binary/xml format without loosing important information.

Because ConnId targets Java 6 and OpenICF try to stay Java 5 compatible I listed here the Java 5 RuntimeExceptions and marked the Java6. I check but I didn't see any new required Java 6 exceptions so I suggest to stay with Java 5.   

I listed and I categorised the exceptions in JVM.

FU - Fully Used: These exceptions should be part of the contract. They are named Known exceptions in the sample project.

NU - Not Used in the contract. If this exception is thrown then it was downcast to plain new RuntimeException(e.getMessage()) 

Smart Message: The exception has more information then just e.getMessage() so concatenating them would contains all information. This would requires additional logic per exception. Q: Does it worth it?

Complex: The Class does not have Default or String constructor.

Null message: The Exception does not have Message (Exception )

Subclasses: The Exception has many other subclasses

AnnotationTypeMismatchException, NU Smart Message

ArithmeticException, NU

ArrayStoreException, NU

BufferOverflowException, --> System.IO.InternalBufferOverflowException NU

BufferUnderflowException,  NU Null message

CannotRedoException, NU Null message

CannotUndoException, NU Null message

ClassCastException, --> System.InvalidCastException NU

CMMException, NU

ConcurrentModificationException,  NU

Java 6 DataBindingException

DOMException,  NU Smart Message

EmptyStackException, NU Null message 

EnumConstantNotPresentException, NU Smart Message

EventException, NU Smart Message

IllegalArgumentException,  --> System.ArgumentException  FU Subclasses

IllegalMonitorStateException, NU

IllegalPathStateException,  NU

IllegalStateException, --> System.InvalidOperationException -- FU/ NU? Subclasses

ImagingOpException, NU

IncompleteAnnotationException, NU Smart Message

IndexOutOfBoundsException, NU

JMRuntimeException, NU

LSException, NU Smart Message

MalformedParameterizedTypeException,  NU Null message

Java 6 MirroredTypeException, 

Java 6 MirroredTypesException 

MissingResourceException, NU Smart Message

NegativeArraySizeException, NU

NoSuchElementException, NU

Java 6 NoSuchMechanismException,

NullPointerException,  --> System.NullReferenceException  FU (don't confuse with System.ArgumentNullException extends System.ArgumentException :( )

ProfileDataException, NU

ProviderException, NU

RasterFormatException, NU 

RejectedExecutionException, NU

SecurityException --> System.Security.SecurityException

SystemException, Too complex, use the message only

Java 6 TypeConstraintException

TypeNotPresentException,  NU, Complex

UndeclaredThrowableException, NU Complex

Java 6 UnknownAnnotationValueException, 

Java 6 UnknownElementException, 

Java 6 UnknownTypeException

UnmodifiableSetException, NU

UnsupportedOperationException, --> System.NotSupportedException FU

Java 6 WebServiceException

Exceptions

AclNotFoundException, NU Null message

ActivationException,NU

AlreadyBoundException,NU

ApplicationException, CORBA Too Complex

AWTException, NU 

BackingStoreException,NU

BadAttributeValueExpException, NU Complex

BadBinaryOpValueExpException, NU Complex 

BadLocationException, NU Complex 

BadStringOperationException,NU

BrokenBarrierException,NU

CertificateException,NU

ClassNotFoundException, NU

CloneNotSupportedException,NU

DataFormatException,NU

DatatypeConfigurationException,NU

DestroyFailedException,NU

ExecutionException,NU

ExpandVetoException, Complex

FontFormatException,NU

GeneralSecurityException,  FU Subclass

GSSException, Too Complex 

IllegalAccessException,NU

IllegalClassFormatException,NU

InstantiationException,NU

InterruptedException,NU

IntrospectionException,EE

InvalidApplicationException, NU Complex

InvalidMidiDataException,NU

InvalidPreferencesFormatException,NU

InvalidTargetObjectTypeException,NU

InvocationTargetException,NU

IOException, NU SUBCLASSES

Java 6 JAXBException, NU Smart Message

JMException,NU

Java 6 KeySelectorException,NU

LastOwnerException, NU Null message

LineUnavailableException,NU

Java 6 MarshalException,NU

MidiUnavailableException,NU

MimeTypeParseException,NU

Java 6 MimeTypeParseException,NU

NamingException, NU Complex Subclasses

NoninvertibleTransformException,NU

NoSuchFieldException, NU 

NoSuchMethodException,NU

NotBoundException,NU

NotOwnerException, NU Null Message

ParseException, NU Complex 

ParserConfigurationException,NU

PrinterException,NU

PrintException,NU

PrivilegedActionException, NU Too Complex

PropertyVetoException, NU Complex

RefreshFailedException,NU

RemarshalException,NU

RuntimeException,  FU Subclasses

SAXException, NU Subclasses

Java 6 ScriptException,NU

ServerNotActiveException,NU

Java 6 SOAPException,NU

SQLException, NU Subclasses

TimeoutException,NU

TooManyListenersException,NU

TransformerException, NU Smart Message

Java 6 TransformException,NU

UnmodifiableClassException,NU

UnsupportedAudioFileException,NU

UnsupportedCallbackException,NU

UnsupportedFlavorException, NU Complex

UnsupportedLookAndFeelException, NU 

Java 6 URIReferenceException, NU Smart Message

URISyntaxException, NU Complex

UserException, NU Corba, Subclasses

XAException,NU

XMLParseException,NU

Java 6 XMLSignatureException,NU

Java 6 XMLStreamException, NU Smart Message

XPathException NU Subclasses

I think this mail is getting long so I close it now and we can discuss your feedbacks and go deeper into some tray areas.

Regards,

Laszlo

[Gael Allioux]

Hi,

I propose another little enhancement that could be convenient when it comes
to configuration UIs and Glossary for connectors:

Add a "description" string property to the AttributeInfo meta data. Quite often (think about SAP for example)
attribute name is not really helpful. Need to think about I18N and l10N as well...

WDYT?

Gael

[Francesco Chicchiriccò]

On 21/08/2013 17:36, Gael Allioux wrote:

Hi,

I propose another little enhancement that could be convenient when it comes
to configuration UIs and Glossary for connectors:

Add a "description" string property to the AttributeInfo meta data. Quite often (think about SAP for example)
attribute name is not really helpful. Need to think about I18N and l10N as well...

WDYT?

+1

Regards.

[Fabio Martelli]

Il 21/08/2013 17:38, Francesco Chicchiriccò ha scritto:

On 21/08/2013 17:36, Gael Allioux wrote:

Hi,

I propose another little enhancement that could be convenient when it comes
to configuration UIs and Glossary for connectors:

Add a "description" string property to the AttributeInfo meta data. Quite often (think about SAP for example)
attribute name is not really helpful. Need to think about I18N and l10N as well...

WDYT?

+1

Nice. +1 as well

Regards.

-- 
Francesco Chicchiriccò

ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/