Re: Discuss: Move InvalidSearchConditionException to common

[Francesco Chicchiriccò <il...@apache.org>]

On 30/01/2013 13:25, Christian Schneider wrote:
> Hi Francesco,
>
> I am currently working on the patch to move the
> InvalidSearchConditionException and make it checked again.
> I am not sure if making it checked is a good idea. As the exception will
> now be part of the service interface I will have to add it to lots of
> methods. None of these methods really handles this exception.
> The reason for this is that before we used the RestTemplate to access
> the services. So it did not matter if the service throws a checked
> exception. We would not be forced to catch it. Now with the interfaces
> for the services we have to add the checked exception to the service
> interface if we want the impl to be able to throw it. So the client is
> also affected by that.

Clear.

> In the end the exception now also appears in AttributableDataProvider
> which can not rethrow the exception as it would violate the
> IDataProvider interface. So I would have to catch it there but I have no
> idea what to do if it happens.

As said before, this should never happen on the admin console since the 
wrapper is architected to generate only valid conditions and to check 
for correctness; anyway, the catch() block should only be able to log 
and report this error to the user.

> I created a gist of the patch so you can have a look:
> https://gist.github.com/4672932
>
> So I see two possible solutions to improve this:
> 1) Make the exception unchecked again so it can be thrown by the
> services but has no effect on the client side
> 2) Catch the exception in the server impls and rethrow it as a
> RuntimeException so it does not affect the clients

If the admin console is the only problem, we can easily find a fix there.

If you still think having this checked is too much problematic for you, 
just leave it unchecked.

Regards.

> On 30.01.2013 08:29, Francesco Chicchiriccò wrote:
>> On 28/01/2013 15:38, Francesco Chicchiriccò wrote:
>>> On 28/01/2013 15:36, Christian Schneider wrote:
>>>> Currently InvalidSearchConditionException is in core.persistence.dao
>>>> and
>>>> is a checked exception.
>>>> If we want to make this exception part of the UserService interface to
>>>> have it available on the client we will have to move it to common.
>>>>
>>>> Currently the console does not seem to use the exception on the client
>>>> side. So my preliminary solution to get it running is to make the
>>>> exception unchecked so I can pass it to the
>>>> exception mapper without having it in the interface.
>>>>
>>>> So basically the two things to discuss are if we want to move the
>>>> exception to common and if we want to make it unchecked. I personally
>>>> would do both.
>>> This exception is not used by the admin console because the code is
>>> done so that no invalid search conditions are sent to the core;
>>> however, other clients are likely to need this exception to be returned.
>>>
>>> Hence: +1 for moving it to common, but don't see enough reason to
>>> make it unchecked.
>> I've noticed that this issue was moved to common as unchecked: any
>> additional reason behind not leaving it checked than the need to
>> change some interface method's signature?
>>
>> Regards.

-- 
Francesco Chicchiriccò

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

Re: Discuss: Move InvalidSearchConditionException to common

[Christian Schneider <ch...@die-schneider.net>]

No problem to make it checked. I just wanted to get feedback first 
before I introduce the exceptions in all the methods. Of course I hoped 
to persuade you to make it unchecked but as this is just my personal 
preference I do not insist on this :-)

As having the exceptions in the client code is ok with you I will move 
the Exception and make it checked again.
I will catch and log the exception in AttributableDataProvider.

Christian

Am 01.02.2013 12:06, schrieb Francesco Chicchiriccò:
> On 30/01/2013 13:25, Christian Schneider wrote:
>> Hi Francesco,
>>
>> I am currently working on the patch to move the
>> InvalidSearchConditionException and make it checked again.
>> I am not sure if making it checked is a good idea. As the exception will
>> now be part of the service interface I will have to add it to lots of
>> methods. None of these methods really handles this exception.
>> The reason for this is that before we used the RestTemplate to access
>> the services. So it did not matter if the service throws a checked
>> exception. We would not be forced to catch it. Now with the interfaces
>> for the services we have to add the checked exception to the service
>> interface if we want the impl to be able to throw it. So the client is
>> also affected by that.
>
> Clear.
>
>> In the end the exception now also appears in AttributableDataProvider
>> which can not rethrow the exception as it would violate the
>> IDataProvider interface. So I would have to catch it there but I have no
>> idea what to do if it happens.
>
> As said before, this should never happen on the admin console since 
> the wrapper is architected to generate only valid conditions and to 
> check for correctness; anyway, the catch() block should only be able 
> to log and report this error to the user.
>
>> I created a gist of the patch so you can have a look:
>> https://gist.github.com/4672932
>>
>> So I see two possible solutions to improve this:
>> 1) Make the exception unchecked again so it can be thrown by the
>> services but has no effect on the client side
>> 2) Catch the exception in the server impls and rethrow it as a
>> RuntimeException so it does not affect the clients
>
> If the admin console is the only problem, we can easily find a fix there.
>
> If you still think having this checked is too much problematic for 
> you, just leave it unchecked.
>
> Regards.

-- 
  
Christian Schneider
http://www.liquid-reality.de

Open Source Architect
Talend Application Integration Division http://www.talend.com