commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [math] Exception Design
Date Thu, 24 Dec 2015 00:41:29 GMT
On Wed, 23 Dec 2015 20:18:10 +0100, Luc Maisonobe wrote:
> Le 23/12/2015 14:32, Gilles a écrit :
>> Hello.
>>
>> On Wed, 23 Dec 2015 10:38:03 +0100, luc wrote:
>>> Le 2015-12-23 01:41, Gilles a écrit :
>>>> On Tue, 22 Dec 2015 13:17:16 -0600, Ole Ersoy wrote:
>>>>> On 12/22/2015 11:46 AM, Gilles wrote:
>>>>>> Hi.
>>>>>> On Mon, 21 Dec 2015 22:44:16 -0600, Ole Ersoy wrote:
>>>>>>> On 12/21/2015 06:44 PM, Gilles wrote:
>>>>>>>> On Mon, 21 Dec 2015 12:14:16 -0600, Ole Ersoy wrote:
>>>>>>>>> Hi,
>>>>>>>>> I was considering jumping into the JDKRandomGenerator

>>>>>>>>> exception
>>>>>>>>> discussion, but I did not want to hijack it.
>>>>>>>>> Not sure if any of you have had a chance to looks at
this:
>>>>>>>>> https://github.com/firefly-math/firefly-math-exceptions/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 
>>>>>>>>> https://github.com/firefly-math/firefly-math-exceptions/blob/master/src/main/java/com/fireflysemantics/math/exception/MathException.java
>>>>>>>>>
>>>>>>>> [...]
>>>>>>>> But I did not see how localization is handled.
>>>>>>> I did leave localization out.  I think localization was a hard
>>>>>>> requirement in earlier versions of CM, but I'm hoping that 
>>>>>>> there is
>>>>>>> some flexibility on this
>>>>>> There was not, since I argued many times to leave it out.
>>>>>> So unless you can show practically how it can work, I have my 
>>>>>> doubts
>>>>>> that we'll be allowed to go forward with this approach.
>>>>>>
>>>>>>> and that future versions can defer to a
>>>>>>> utility that uses the ExceptionTypes Enum instance as the key

>>>>>>> to look
>>>>>>> up the internationalized template string.
>>>>>> Looks good.  Where is the code? ;-)
>>>>> So CM clients would:
>>>>> catch(MathException e) {
>>>>>     String exceptionTemplate =
>>>>> ResourceBundle.getBundle("cm.exception.templates", new 
>>>>> Locale("en",
>>>>> "US")).getString(e.getType());
>>>>>     String i18Nmessage = buildMessage(exceptionTemplate,
>>>>> e.getContext());
>>>>>     ...
>>>>> }
>>>>> I can prototype that out more.  Just trying to get a feel for how
>>>>> viable the concept is first.
>>>> I'm not sure I understand correctly.
>>>> Does that mean that
>>>> 1. Uncaught exceptions will lead to a message in English?
>>>> 2. Every "catch" must repeat the same calls (although the 
>>>> arguments
>>>> are likely
>>>>    to be the same for all clauses (and for all applications)?
>>>> Comparing this with the current behaviour (where the translated
>>>> message String
>>>> is created when "e.getLocalizedMessage()" is called) is likely to
>>>> make people
>>>> unhappy.
>>>
>>> This could be made simpler with some task delegating between user
>>> code and CM code.
>>> What about :
>>>
>>>  interface ExceptionLocalizer {
>>>     /** Localize an exception message.
>>>       * @param locale locale to use
>>>       * @param me exception to localize
>>>       * @return localized message for the exception
>>>       */
>>>     String localize(Locale locale, MathException me);
>>>  }
>>>
>>> and having ExceptionFactory hold a user-provided implementation of
>>> this interface?
>>>
>>>  public class ExceptionFactory {
>>>
>>>    private static ExceptionLocalizer localizer = new 
>>> NoOpLocalizer();
>>>
>>>    public static setLocalizer(ExceptionLocalizer l) {
>>>       localizer = l;
>>>    }
>>
>> I think that this is potentially dangerous for two reasons (if I'm
>> not mistaken): it's not thread-safe and it can be called by any
>> library used by the main application.
>
> Intermedaite libraries could also call it, and I hope they would be
> designed to use a consistent way for their own exception (they should
> let the user specify the localization mechanism, use it for 
> themselves
> and pass the configuration to CM).

I'm not sure I follow.

IIUC the implications, libraries should be forbidden to call a method
such as "setLocalizer".

In fact "localizer" should be final. [It could be initialized in a way
similar to what is done by "slf4j".  But this is a lot of work not 
worth
it if we can drop direct support for localiszation in CM.]

>
> Thread safety here is really not a concern (but we could protect it
> if desired). This setting is a configuration level setting, it is
> usually done once near the beginning of the main program. You don't
> change the mechanism every millisecond.
>
>>
>> I think that the "localizer" instance must be obtained in a way 
>> which
>> the end-user controls.
>
> The user controls it. The setLocalizer method can be called directly 
> by
> user, and in fact I expect the user to do it.

The user is not in control because any code he calls can override the
setting.

>>>
>>>    public static String localize(Locale locale, MathException me) {
>>>      return localizer.localize(locale, me);
>>>    }
>>>
>>>    /** Default implementation of the localizer that does nothing. 
>>> */
>>>    private static class NoOpLocalizer implements ExceptionLocalizer 
>>> {
>>>           /** {@inheritDoc} */
>>>           @Override
>>>           public String localize(MathException me) {
>>>            return me.getMessage();
>>>          }
>>>    }
>>>
>>>  }
>>>
>>> and MathException could implement both getLocalizedMessage() and
>>> even getMessage(Locale) by delegating to the user code:
>>>
>>>   public class MathException {
>>>
>>>     public String getLocalizedMessage() {
>>>       return ExceptionFactory.localize(Locale.getDefault(), this);
>>>     }
>>>
>>>     public String getMessage(Locale locale) {
>>>       return ExceptionFactory.localize(locale, this);
>>>     }
>>>
>>>     ...
>>>
>>>   }
>>>
>>> One thing that would be nice would be that in addition to the get 
>>> method,
>>> MathException also provides a getKeys to retrieve all keys and a 
>>> getType
>>> to retrieve the type. The later correspond to the getPatern (or
>>> getLocalizable)
>>> I asked for a few years ago in ExceptionContext. The point for 
>>> these
>>> methods
>>> is that if we provide users a way to retrieve the parameters that 
>>> were
>>> used
>>> in the exception construction, then we can delegate localization to 
>>> users
>>> as they can do their own code that will rebuild a complete meaasage 
>>> as
>>> they
>>> want. When you have only the keys and they have reused names like
>>> OPERATOR
>>> or VECTOR, it can't be delegated.
>>
>> If those are available (as suggested in Ole's example above), would 
>> you
>> indeed
>> be OK that localization is not a CM concern anymore?
>
> Yes at the condition that user can use it to implement something like
> the ExceptionLocalizer interface and this interface is already 
> supported
> by CM exceptions to implement getLocalizedMessage (at least). The
> getLocalizedMessage is a standard method inherited directly from
> Throwable, it is not an addition from us (on the other hand
> getMessage(Locale) *is* an extension I promoted).

I don't follow: I thought that if Ole's "MathException" provides 
"getType",
"getKeys" and ("getValues" ?), then you have the same building blocks
to define a custom formatting (and localization).

By "dropping support", I mean dropping support. ;-)
I.e. no more "Localize..." classes under "org.apache.commons.math4"

The list of translated type could still be maintained here, in the same
way the unit tests and user guide are.

> This getLocalizedMessage is the standard way many existing frameworks
> use to display the message to end users, and we can't change this
> behaviour to have them call the user localization code. So this user
> localization code must lie somewhere beneath the standard
> getLocalizedMessage call. The proposal above allow to divert it to
> user code with CM providing only the required plumbing (but it must
> still provide the plumbing).
>
>>
>> We could provide code of how to perform the translation in the 
>> userguide.
>
> Yes.
>
>>
>>> Note that this is independent of the fact there is one or several
>>> hierarchies.
>>> If there are several ones, the two one-liners above must simply be 
>>> copied
>>> (yeah, code duplication, I know).
>>>
>>>>
>>>>>>>>
>>>>>>>>> I think it satisfies everyone's requirements with:
>>>>>>>>> - A single MathException (No hierarchy)
>>>>>>>> That would not satisfy everyone. :-!
>>>>>>>>
>>>>>>>>> - The ExceptionTypes Enum contains all the exception
types
>>>>>>>>> - The ExceptionTypes Enum 'key' maps to the corresponding
>>>>>>>>> message 1 to 1
>>>>>>>>> - The ExceptionFactory (Utility) throws exceptions, if

>>>>>>>>> necessary,
>>>>>>>>> that have always have a single unique root cause, such
as 
>>>>>>>>> NPEs
>>>>>>>> I was wondering whether the "factory" idea could indeed 
>>>>>>>> satisfy
>>>>>>>> everyone.
>>>>>>>> Rather than throwing the non-standard "MathException", the
>>>>>>>> factory would
>>>>>>>> generate one of the standard exceptions, constructed with
the
>>>>>>>> internal
>>>>>>>> "MathException" as its cause:
>>>>>>> I think it's good that CM throws CM specific exceptions.  This

>>>>>>> way
>>>>>>> when I write the handler I can know that the exception is CM

>>>>>>> specific
>>>>>>> without having to unwrap it.
>>>>>> But if there are several CM exceptions hierarchies, the handler
>>>>>> will have
>>>>>> to check for every base type, leading to more code.
>>>>> True dat - but if there are no CM exception hierarchies then they
>>>>> don't :).
>>>> I'd be interested in settling the matter of whether we must use a 
>>>> single
>>>> hierarchy because of technical limitations, or if it is a good 
>>>> solution
>>>> on its own, i.e. extending standard exceptions is not a better 
>>>> practice
>>>> after all.
>>>>
>>>>>> We could provide a utility:
>>>>>> public boolean isMathException(RuntimeException e) {
>>>>>>   if (e instanceof MathException) {
>>>>>>     return true;
>>>>>>   }
>>>>>>   final Throwable t = e.getCause();
>>>>>>   if (t != null) {
>>>>>>     if (e instanceof MathException) {
>>>>>>       return true;
>>>>>>     }
>>>>>>   }
>>>>>>   return false;
>>>>>> }
>>>>> Or just not wrap.
>>>> Of course, but choosing one or the other is not a technical 
>>>> problem;
>>>> it's design decision.  Do we have arguments (or reference to 
>>>> them)?
>>>>
>>>>>>>> public class ExceptionFactory {
>>>>>>>> public static void throwIllegalArgumentException(MathException

>>>>>>>> e) {
>>>>>>>>     throw new IllegalArgumentException(e);
>>>>>>>>   }
>>>>>>>> public static void throwNullPointerException(MathException
e) 
>>>>>>>> {
>>>>>>>>     throw new NullPointerException(e);
>>>>>>>>   }
>>>>>>>> // And so on...
>>>>>>>> }
>>>>>>>> So, CM code would be
>>>>>>>> public class Algo {
>>>>>>>> public void evaluate(double value) {
>>>>>>>>     // Check precondition.
>>>>>>>>     final double min = computeMin();
>>>>>>>>     if (value < min) {
>>>>>>>>       final MathException e = new
>>>>>>>> MathException(NUMBER_TOO_SMALL).put(CONSTRAINT, 
>>>>>>>> min).put(VALUE,
>>>>>>>> value);
>>>>>>>>       ExceptionFactory.throwIllegalArgumentException(e);
>>>>>>>>     }
>>>>>>>> // Precondition OK.
>>>>>>>>   }
>>>>>>>> }
>>>>>>> Another thing that I hinted to is that the the factory builds

>>>>>>> in the
>>>>>>> precondition check in the throw method.  So that the line:
>>>>>>> if (value < min) {
>>>>>>> can be nixed.
>>>>>> It seems nice to ensure that the exception raised is consistent
>>>>>> with the
>>>>>> checked condition.
>>>>> That's the idea.
>>>> OK, but do you foresee that all precondition checks will be handle 
>>>> by
>>>> factory methods.
>>>> It would not be so nice to have explicit checks sprinkled here and
>>>> there.
>>>>
>>>>>> Then, a factory method like
>>>>>>   throwNotStrictlyPositiveException(Number value, String key)
>>>>>> should probably be renamed to
>>>>>>   checkNotStrictlyPositiveException(Number value, String key)
>>>>> 'check' is good.  I'm going to change it to check.
>>>
>>> as the name was changed to checkSomething, the last part Exception 
>>> in
>>> the name could be dropped.
>>>
>>>>>> Also, shouldn't the "key" argument should be optional?
>>>>> The key is used to initialize the exception context with the 
>>>>> Number
>>>>> instance.  Different modules could have different keys.  For 
>>>>> example
>>>>> the Arithmetic module has keys X and Y.  So if Y caused the 
>>>>> exception
>>>>> then Y would be passed as the key.  So if we are checking both we
>>>>> would do this:
>>>>> checkNotStrictlyPositiveException(x, X);
>>>>> checkNotStrictlyPositiveException(y, Y);
>>>>>>
>>>>>>>> Then, in an application's code:
>>>>>>>> public void appMethod() {
>>>>>>>>   // ...
>>>>>>>> // Use CM.
>>>>>>>>   try {
>>>>>>>>     Algo a = new Algo();
>>>>>>>>     a.evaluate(2);
>>>>>>>>   } catch (IllegalArgumentException iae) {
>>>>>>>>     final Throwable cause = iae.getCause();
>>>>>>>>     if (cause instanceof MathException) {
>>>>>>>>       final MathException e = (MathException) cause;
>>>>>>>> // Rethrow an application-specific exception that will make

>>>>>>>> more
>>>>>>>> sense
>>>>>>>>       // to my customers.
>>>>>>>>       throw new InvalidDataInputException(e.get(CONSTRAINT),
>>>>>>>> e.get(VALUE));
>>>>>>>>     }
>>>>>>>>   }
>>>>>>>> }
>>>>>>>> This is all untested.
>>>>>>>> Did I miss something?
>>>
>>> In the code above, if the iae does not have a cause, or if it is 
>>> not
>>> a MathException,
>>> the iae should be rethrown.
>>
>> Indeed!
>> The updated code (also unstested):
>>
>>    try {
>>      Algo a = new Algo();
>>      a.evaluate(2);
>>    } catch (IllegalArgumentException iae) {
>>      final MathException e = ExceptionFactory.getMathException(iae);
>>
>>      if (e != null) {
>>        // Rethrow an application-specific exception that will make 
>> more
>> sense
>>        // to my customers.
>>        throw new InvalidDataInputException(e.get(CONSTRAINT),
>> e.get(VALUE));
>>      } else {
>>        throw iae;
>>      }
>>    }
>>
>>>>>>> I think you got it all...But the handler will be shorter if the
>>>>>>> exception is not wrapped.
>>>>>> But not significantly, I guess.
>>>>>> We could also provide
>>>>>> public MathException getMathException(RuntimeException e) {
>>>>>>   if (e instanceof MathException) {
>>>>>>     return (MathException) e;
>>>>>>   }
>>>>>>   final Throwable t = e.getCause();
>>>>>>   if (t != null) {
>>>>>>     if (e instanceof MathException) {
>>>>>>       return (MathException) e;
>>>>>>     }
>>>>>>   }
>>>>>>   return null;
>>>>>> }
>>>>>> And then define the other utility as:
>>>>>> public boolean isMathException(RuntimeException e) {
>>>>>>   return getMathException(e) != null;
>>>>>> }
>>>>>>
>>>>>>> The pattern I'm used to is that libraries
>>>>>>> wrap the exceptions of other libraries in order to offer a
>>>>>>> standardized facade to the user.  For example Spring wraps 
>>>>>>> Hibernate
>>>>>>> exceptions, since Spring is a layer on top of Hibernate and 
>>>>>>> other
>>>>>>> data
>>>>>>> access providers.
>>>>>> What do they advertize?  Standard exception, possibly extended, 
>>>>>> or
>>>>>> specific ones, possibly belonging to single hierarchy?
>>>>> Spring namespaced - single hierarchy:
>>>>>
>>>>> 
>>>>> http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/dao/DataAccessException.html
>>>>>
>>>>> BTW - this is blue sky thinking - but Spring Boot has an
>>>>> @ExceptionHandler annotation that allows the developer to wire up 
>>>>> an
>>>>> exception handler.  It might be worth exploring something similar 
>>>>> for
>>>>> the purpose of automating I18N requirements.
>>>>> @ExceptionHandler(MathException.class)
>>>>> someClientCodeThatUsesCM();
>>>>
>>>> That would be quite necessary I'm afraid. ;-)
>>>
>>> Not necessarily. The above support for I18N is quite simple.
>>
>> Maybe too simple... ;-)
>> [What did they say about global variables?]
>
> If the static fields hurts your feelings,

It's not just that.

> we can always hide
> it using an official design pattern like the singleton for
> ExceptionFactory, and even use the Initialization-on-demand
> holder idiom to store the singleton. Then the localizer would
> be an instance field and there would be a static getInstance()
> method in the factory. Well, a singleton design pattern plus
> a IODH idiom design pattern are really only a global variable
> in a pretty dress, so it is only hiding the fact.

Unless I'm mistaken, other singletons in CM are initialized by CM,
and cannot be changed afterwards.

> At the root, yes global variables are frowned upon, but they
> do have some use for configuration and making sure some
> configuration data can be retrieved from everywhere reliably.
> There are many other places were global variables are used in
> Java. Just to mention one example that is deirectly related
> to our topic, Locale.getDefault() and Locale.setDefault() are
> semantically really accesses to a global variable too.

I think that this is not something to introduce lightly.
When/if we'll want to explore multi-threading, it will add to
the problems.
If the localization feature can be achieved without resorting
to a global setting, we should favour it.

Best,
Gilles

>
> best regards,
> Luc
>
>>
>> Best regards,
>> Gilles
>>
>>>
>>> best regards,
>>> Luc
>>>
>>>> Best regards,
>>>> Gilles
>>>>
>>>>> Cheers,
>>>>> Ole


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message