Project

General

Profile

DynamicRosterIfc change request

John Catron
Added over 5 years ago

Hi!

We have been using the DynamicRosterIfc and it has done great to serve our needs as far as the ability to supply our users with a common roster.

However, we have run into an issue where we need the interface to be a bit more granular with errors. Specifically, it would be useful if the methods in the DynamicRosterIfc interface didn't limit us to only the 'NotAuthorizedException' since we have determined we have 2 exception cases when fetching a DynamicRoster.

The first case is that we are attempting to fetch a DynamicRoster and the client is not allowed. The NotAuthorizedException matches this case.

The second case is when there is an error when retrieving the DynamicRoster information itself. This can happen if the database or storage of the DynamicRoster fails to respond correctly, or returns malformed data. We are currently given no path to recognize this error and are forced to tell the user they are unauthorized essentially logging them out.

Our team would prefer to throw a 'ContactRetrievalException' that would be logged in our error logs but will still return an empty roster to the user. We understand that we could do this by using RuntimeExceptions so that it doesn't violate the interface, but we think the interface would be better to allow this acceptable exception case.

The entire point of a DynamicRoster is the server is supplying you with this set of buddies which means by its very definition it has to retrieve them or build them somehow. It feels strange that we have no way to express that retrieval has failed. Because if we use the NotAuthorizedException then they are logged out and they shouldn't be since it is still possible to retrieve their standard buddy list.


Replies (6)

Avatar?id=6023&size=32x32

Added by Artur Hefczyc TigaseTeam over 5 years ago

I get your point. I think adding some kind of roster repository access exception makes sense. Extending the API is possible but please explain to me my below concerns. I just want to make sure I understand the problem you are facing.

  1. Why your DynamicRosterIfc implementation cannot log the contact retrieval error and return just an empty roster set?

  2. Assuming I add a new exception allowed to be thrown from the implementation, you probably want me also to add a specific handling for the exception. There are a few possibilities:

    1. Record the error in a log file and return to a user an empty roster
    2. Record the error in a log file and return to a user roster error

Added by John Catron over 5 years ago

You have a fair point with the log and return empty set idea. We were thinking that it'd be better for the DynamicRoster to throw an error, however, and allow the JabberIqRoster to determine whether an error should be thrown to user or to use empty set for DynamicRoster.

DynamicRoster has no way to know whether or not you have a regular roster. If you have no regular roster and DynamicRoster fails then we feel it is fitting for the server to return an error to the user since their chat service is useless without a roster. Whereas if the DynamicRoster fails but they DO have a regular roster they could continue to use chat just as a slightly degraded service since the convenience of the given roster isn't there.

So as for your error handling cases we were thinking case #1 if the user has another form of roster. Then case #2 would be useful if DynamicRoster was there only source of contacts.

Added by John Catron over 5 years ago

Update: After speaking with a co-worker I think we feel more like Case #1 should always be used since DynamicRoster is a server/admin only concept so there is no reason to notify the user that it failed. Users have no way to know if/when they will ever receive a DynamicRoster so they have no reason to know when it fails.

We still feel, however, that the exception handling belongs in the handler above DynamicRoster because that is more of a usability decision. The decision to throw an error or not throw an error is a question of what users would want and that type of logic doesn't really belong in DynamicRoster it solely exists to build the roster. To us it feels more like the JabberIqRoster or whatever is handling the mediation between the server and the user when they request the roster is more fit to handle that decision.

For example if I have multiple dynamic roster sources or if I ever change it in the future that doesn't change whether or not a user would want an error. If i want user to receive an error instead of an empty list I would expect to replace the plugin that actually communicates with the user rather than the source of the data.

Avatar?id=6023&size=32x32

Added by Artur Hefczyc TigaseTeam over 5 years ago

OK, this makes sense to me. My hesitation to handle exceptions from DynamicRoster in JabberIqRoster is that DynamicRoster is a third-party implementation. Therefore, JabberIqRoster has no understanding of the logic and conditions in the DynamicRoster implementation and it is difficult to make a good decision how to react on the exception.

However, this can be controlled through a few different exceptions I suppose:

  1. Empty dynamic roster set - no error

  2. RosterRetrievingException - returns an error to the user

  3. RepositoryAccessException - no error sent to the end-user and an empty dynamic roster generated for the end-user

  4. Any other suggestions for exceptions and handling?

Added by John Catron over 5 years ago

No those 3 sound perfect to handle the use cases we were thinking of. I certainly understand the hesitation on handling exceptions in the JabberIqRoster class but I think with use of DynamicRosterIfc you can mitigate the issues and limit them to only exceptions that you can handle.

Thanks so much for looking into all of this it means a lot!

Avatar?id=6023&size=32x32

Added by Artur Hefczyc TigaseTeam over 5 years ago

Task added: #1616

    (1-6/6)