|
From: Ben A. <ben...@ac...> - 2007-01-19 06:56:24
Attachments:
patch.txt
|
Hi all Please find attached another patch. It fixes a bug introduced in recent changes to ContainerCopier. It was the same bug we saw earlier this month, being building with Ant requires casting ternary operators. I've also modified BaseTransformer to NOT wrap RuntimeExceptions in a TransformationException. This is really critical for us. If we have a mutator method throw SomeBusinessException which extends RuntimeException, it's necessary that SomeBusinessException is what Morph eventually returns - not wrapped in a TransformationException. There's no technical need for it in the case of a RuntimeException, so that attached patch provides this enhancement. I hope you agree with this change. Cheers Ben |
|
From: Alan S. <ala...@op...> - 2007-01-19 11:12:36
|
Hi, Ben's patch to allow us to throw runtime business exceptions from our domain objects and not have them wrapped as TransformationExceptions. I tested it today and all works fine. I hope this patch and all other of Ben's recent suggestions and changes can make it into the next release of Morph. Cheers Alan |
|
From: Matt S. <Mat...@wh...> - 2007-01-19 17:00:59
|
Hi Ben -
Before adopting the change to not wrap RuntimeExceptions as
TransformationExceptions, I would like to discuss the pros and cons of
this change in greater detail. To me, the benefit of wrapping these
exceptions as TransformationExceptions is that higher up the stack you
know that an error occurred while a Morph transformation was being
performed, as opposed to an error happening in Hibernate or some other
library or an error happening in higher level objects that are directly
servicing client requests (MVC controllers, Swing Actions, etc)
I can understand that if you've gone to the trouble of introducing your
own business exceptions that you want to work with those exceptions
rather than Morph's exceptions. However, couldn't you just catch the
MorphException and retrieve its cause with a call to getCause() ? Or is
the issue that in a nested transformation you end up with your business
exceptions being buried 3 or 4 exceptions deep? If that's the problem,
I propose what we should do is change Morph so that it doesn't wrap its
own exceptions 3 or 4 deep, or that if it does do so, it at least has an
easy way to reference the first original non-Morph exception. Perhaps
we could introduce a new method called getFirstCause or something similar.
Matt S
Ben Alex wrote:
> Hi all
>
> Please find attached another patch.
>
> It fixes a bug introduced in recent changes to ContainerCopier. It was
> the same bug we saw earlier this month, being building with Ant requires
> casting ternary operators.
>
> I've also modified BaseTransformer to NOT wrap RuntimeExceptions in a
> TransformationException. This is really critical for us. If we have a
> mutator method throw SomeBusinessException which extends
> RuntimeException, it's necessary that SomeBusinessException is what
> Morph eventually returns - not wrapped in a TransformationException.
> There's no technical need for it in the case of a RuntimeException, so
> that attached patch provides this enhancement. I hope you agree with
> this change.
>
> Cheers
> Ben
>
> ------------------------------------------------------------------------
>
>
> Property changes on: /home/balex/workspace/morph
> ___________________________________________________________________
> Name: svn:ignore
> +
> dist
> target
> build
>
>
> Index: /home/balex/workspace/morph/src/core/net/sf/morph/transform/copiers/ContainerCopier.java
> ===================================================================
> --- /home/balex/workspace/morph/src/core/net/sf/morph/transform/copiers/ContainerCopier.java (revision 94)
> +++ /home/balex/workspace/morph/src/core/net/sf/morph/transform/copiers/ContainerCopier.java (working copy)
> @@ -207,7 +207,7 @@
> // final Iterator that will be returned to the user of the
> // ContainerCopier
> Iterator iterator = getContainerReflector().getIterator(source);
> - return iter ? iterator : new IteratorEnumeration(iterator);
> + return iter ? iterator : (Object) new IteratorEnumeration(iterator);
> }
> return super.convertImpl(destinationClass, source, locale);
> }
> Index: /home/balex/workspace/morph/src/core/net/sf/morph/transform/transformers/BaseTransformer.java
> ===================================================================
> --- /home/balex/workspace/morph/src/core/net/sf/morph/transform/transformers/BaseTransformer.java (revision 94)
> +++ /home/balex/workspace/morph/src/core/net/sf/morph/transform/transformers/BaseTransformer.java (working copy)
> @@ -127,8 +127,9 @@
> }
> catch (TransformationException e) {
> throw e;
> - }
> - catch (StackOverflowError e) {
> + } catch (RuntimeException e) {
> + throw (RuntimeException) e;
> + } catch (StackOverflowError e) {
> throw new TransformationException(
> "Stack overflow detected. This usually occurs when a transformer implements "
> + ObjectUtils.getObjectDescription(ExplicitTransformer.class)
> @@ -234,8 +235,9 @@
> }
> catch (TransformationException e) {
> throw e;
> - }
> - catch (Exception e) {
> + } catch (RuntimeException e) {
> + throw (RuntimeException) e;
> + } catch (Exception e) {
> throw new TransformationException(
> "Could not initialize transformer "
> + ObjectUtils.getObjectDescription(this), e);
> @@ -296,11 +298,11 @@
>
> try {
> return convertImpl(destinationClass, source, locale);
> - }
> - catch (TransformationException e) {
> + } catch (TransformationException e) {
> throw (TransformationException) e;
> - }
> - catch (Throwable t) {
> + } catch (RuntimeException e) {
> + throw (RuntimeException) e;
> + } catch (Throwable t) {
> if (isTransformable(destinationClass, ClassUtils.getClass(source))) {
> throw new TransformationException(destinationClass, source, t);
> }
> @@ -392,8 +394,9 @@
> }
> catch (TransformationException e) {
> throw e;
> - }
> - catch (Exception e) {
> + } catch (RuntimeException e) {
> + throw (RuntimeException) e;
> + } catch (Exception e) {
> if (isTransformable(destination.getClass(), source.getClass())) {
> throw new TransformationException("Error copying source "
> + ObjectUtils.getObjectDescription(source) + " to destination "
>
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys - and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> ------------------------------------------------------------------------
>
> _______________________________________________
> morph-developer mailing list
> mor...@li...
> https://lists.sourceforge.net/lists/listinfo/morph-developer
>
|
|
From: Alan S. <ala...@op...> - 2007-01-19 23:30:43
|
Hi Matt,
We're using Morph now at Woolworths as the "Assembler" implementation in
ROO to transform between rich domain object and DTO. If I have to catch
a TransformationException (that wraps our business exception) in an MVC
controller, it means we're tied to that implementation. If for what ever
reason we swapped Dozer or a different implementation back in as the
Assembler, then the controllers break. I believe architecturally it
would be more pure not to have the Morph exceptions in client code.
Regards
Alan
Matt Sgarlata wrote:
> Hi Ben -
>
> Before adopting the change to not wrap RuntimeExceptions as
> TransformationExceptions, I would like to discuss the pros and cons of
> this change in greater detail. To me, the benefit of wrapping these
> exceptions as TransformationExceptions is that higher up the stack you
> know that an error occurred while a Morph transformation was being
> performed, as opposed to an error happening in Hibernate or some other
> library or an error happening in higher level objects that are
> directly servicing client requests (MVC controllers, Swing Actions, etc)
>
> I can understand that if you've gone to the trouble of introducing
> your own business exceptions that you want to work with those
> exceptions rather than Morph's exceptions. However, couldn't you just
> catch the MorphException and retrieve its cause with a call to
> getCause() ? Or is the issue that in a nested transformation you end
> up with your business exceptions being buried 3 or 4 exceptions deep?
> If that's the problem, I propose what we should do is change Morph so
> that it doesn't wrap its own exceptions 3 or 4 deep, or that if it
> does do so, it at least has an easy way to reference the first
> original non-Morph exception. Perhaps we could introduce a new method
> called getFirstCause or something similar.
>
> Matt S
>
> Ben Alex wrote:
>> Hi all
>>
>> Please find attached another patch.
>>
>> It fixes a bug introduced in recent changes to ContainerCopier. It was
>> the same bug we saw earlier this month, being building with Ant requires
>> casting ternary operators.
>>
>> I've also modified BaseTransformer to NOT wrap RuntimeExceptions in a
>> TransformationException. This is really critical for us. If we have a
>> mutator method throw SomeBusinessException which extends
>> RuntimeException, it's necessary that SomeBusinessException is what
>> Morph eventually returns - not wrapped in a TransformationException.
>> There's no technical need for it in the case of a RuntimeException, so
>> that attached patch provides this enhancement. I hope you agree with
>> this change.
>>
>> Cheers
>> Ben
>>
>> ------------------------------------------------------------------------
>>
>>
>> Property changes on: /home/balex/workspace/morph
>> ___________________________________________________________________
>> Name: svn:ignore
>> +
>> dist
>> target
>> build
>>
>>
>> Index: /home/balex/workspace/morph/src/core/net/sf/morph/transform/copiers/ContainerCopier.java
>> ===================================================================
>> --- /home/balex/workspace/morph/src/core/net/sf/morph/transform/copiers/ContainerCopier.java (revision 94)
>> +++ /home/balex/workspace/morph/src/core/net/sf/morph/transform/copiers/ContainerCopier.java (working copy)
>> @@ -207,7 +207,7 @@
>> // final Iterator that will be returned to the user of the
>> // ContainerCopier
>> Iterator iterator = getContainerReflector().getIterator(source);
>> - return iter ? iterator : new IteratorEnumeration(iterator);
>> + return iter ? iterator : (Object) new IteratorEnumeration(iterator);
>> }
>> return super.convertImpl(destinationClass, source, locale);
>> }
>> Index: /home/balex/workspace/morph/src/core/net/sf/morph/transform/transformers/BaseTransformer.java
>> ===================================================================
>> --- /home/balex/workspace/morph/src/core/net/sf/morph/transform/transformers/BaseTransformer.java (revision 94)
>> +++ /home/balex/workspace/morph/src/core/net/sf/morph/transform/transformers/BaseTransformer.java (working copy)
>> @@ -127,8 +127,9 @@
>> }
>> catch (TransformationException e) {
>> throw e;
>> - }
>> - catch (StackOverflowError e) {
>> + } catch (RuntimeException e) {
>> + throw (RuntimeException) e;
>> + } catch (StackOverflowError e) {
>> throw new TransformationException(
>> "Stack overflow detected. This usually occurs when a transformer implements "
>> + ObjectUtils.getObjectDescription(ExplicitTransformer.class)
>> @@ -234,8 +235,9 @@
>> }
>> catch (TransformationException e) {
>> throw e;
>> - }
>> - catch (Exception e) {
>> + } catch (RuntimeException e) {
>> + throw (RuntimeException) e;
>> + } catch (Exception e) {
>> throw new TransformationException(
>> "Could not initialize transformer "
>> + ObjectUtils.getObjectDescription(this), e);
>> @@ -296,11 +298,11 @@
>>
>> try {
>> return convertImpl(destinationClass, source, locale);
>> - }
>> - catch (TransformationException e) {
>> + } catch (TransformationException e) {
>> throw (TransformationException) e;
>> - }
>> - catch (Throwable t) {
>> + } catch (RuntimeException e) {
>> + throw (RuntimeException) e;
>> + } catch (Throwable t) {
>> if (isTransformable(destinationClass, ClassUtils.getClass(source))) {
>> throw new TransformationException(destinationClass, source, t);
>> }
>> @@ -392,8 +394,9 @@
>> }
>> catch (TransformationException e) {
>> throw e;
>> - }
>> - catch (Exception e) {
>> + } catch (RuntimeException e) {
>> + throw (RuntimeException) e;
>> + } catch (Exception e) {
>> if (isTransformable(destination.getClass(), source.getClass())) {
>> throw new TransformationException("Error copying source "
>> + ObjectUtils.getObjectDescription(source) + " to destination "
>>
>> ------------------------------------------------------------------------
>>
>> -------------------------------------------------------------------------
>> Take Surveys. Earn Cash. Influence the Future of IT
>> Join SourceForge.net's Techsay panel and you'll get the chance to share your
>> opinions on IT & business topics through brief surveys - and earn cash
>> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> morph-developer mailing list
>> mor...@li...
>> https://lists.sourceforge.net/lists/listinfo/morph-developer
>>
>
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys - and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> ------------------------------------------------------------------------
>
> _______________________________________________
> morph-developer mailing list
> mor...@li...
> https://lists.sourceforge.net/lists/listinfo/morph-developer
>
|
|
From: Ben A. <ben...@ac...> - 2007-01-19 23:53:05
|
Alan Stewart wrote: > We're using Morph now at Woolworths as the "Assembler" implementation in > ROO to transform between rich domain object and DTO. If I have to catch > a TransformationException (that wraps our business exception) in an MVC > controller, it means we're tied to that implementation. If for what ever > reason we swapped Dozer or a different implementation back in as the > Assembler, then the controllers break. I believe architecturally it > would be more pure not to have the Morph exceptions in client code. Hi Matt and Alan Pros of keeping TransformationException wrapping RuntimeExceptions: * Can determine Morph called the method which caused the exception. Mitigating factors: (i) introspect the stack trace and find it out that way; (ii) not a common requirement in my experience to know this anyway. Cons of keeping TransformationException wrapping RuntimeExceptions: * Morph leaks out beyond the assembly layer, as you are *forced* to catch a Morph-specific exception type in order to extract the nested RuntimeException. Mitigating factor: at least you can put such code into a static method. * Try...Catch blocks become more lengthy and complex, as they need to handle the TransformationException in a special manner (either directly or via a static method). * Becomes harder to debug and figure out what is happening. Having the untouched RuntimeException propagate through the control flow is the simplest solution. This means less user support from people unable to figure out what went wrong and are incorrectly attributing failure to Morph or trying to adjust configuration settings to fix what on the surface appears like a "Morph related problem". Less community support will be necessary if TransformationExceptions don't wrap RuntimeException, especially given general moves to RuntimeExceptions means a good number of users will be employing them anyway (and those employing checked exceptions will reasonably expect to have to deal with a nested exception - a side-effect of their checked exception choice). * Correct me if I am wrong, but Dozer doesn't force you to deal with its own exception hierarchy. Alan can confirm this, but I'm 99% sure. So on a competitive level, it seems desirable to follow the lead of the bigger project if users are ever expected to easily transition from Dozer. I may have missed something. Look forward to your thoughts. Cheers Ben |
|
From: Matt S. <Mat...@wh...> - 2007-01-20 01:02:48
|
Thanks for the detailed explanations. I am leaning towards accepting
the patches but I would like to sleep on it.
One situation I was considering is if you are doing something simple
like Morph.convertToInteger("13asdf") you want a TransformationException
instead of a JDK NumberFormatException. This would be in the interest
of hiding implementation details of a particular transformer from a
user. Fortunately, it appears the transformers happen to be written
such that this is exactly what happens even with your change. Of course
I will need to do testing to verify this is the case.
I also like the idea of "if it's a Morph error, throw a Morph exception,
otherwise rethrow the exception that actually occurred". When I first
started really using Morph that was one of the things that confused me
too, even though I wrote the framework: I would see MorphExceptions and
think it was a bug in Morph when in reality the issue lied elsewhere.
Matt
Ben Alex wrote:
> Alan Stewart wrote:
>
>> We're using Morph now at Woolworths as the "Assembler" implementation in
>> ROO to transform between rich domain object and DTO. If I have to catch
>> a TransformationException (that wraps our business exception) in an MVC
>> controller, it means we're tied to that implementation. If for what ever
>> reason we swapped Dozer or a different implementation back in as the
>> Assembler, then the controllers break. I believe architecturally it
>> would be more pure not to have the Morph exceptions in client code.
>>
>
> Hi Matt and Alan
>
> Pros of keeping TransformationException wrapping RuntimeExceptions:
>
> * Can determine Morph called the method which caused the exception.
> Mitigating factors: (i) introspect the stack trace and find it out that
> way; (ii) not a common requirement in my experience to know this anyway.
>
> Cons of keeping TransformationException wrapping RuntimeExceptions:
>
> * Morph leaks out beyond the assembly layer, as you are *forced* to
> catch a Morph-specific exception type in order to extract the nested
> RuntimeException. Mitigating factor: at least you can put such code into
> a static method.
>
> * Try...Catch blocks become more lengthy and complex, as they need to
> handle the TransformationException in a special manner (either directly
> or via a static method).
>
> * Becomes harder to debug and figure out what is happening. Having the
> untouched RuntimeException propagate through the control flow is the
> simplest solution. This means less user support from people unable to
> figure out what went wrong and are incorrectly attributing failure to
> Morph or trying to adjust configuration settings to fix what on the
> surface appears like a "Morph related problem". Less community support
> will be necessary if TransformationExceptions don't wrap
> RuntimeException, especially given general moves to RuntimeExceptions
> means a good number of users will be employing them anyway (and those
> employing checked exceptions will reasonably expect to have to deal with
> a nested exception - a side-effect of their checked exception choice).
>
> * Correct me if I am wrong, but Dozer doesn't force you to deal with its
> own exception hierarchy. Alan can confirm this, but I'm 99% sure. So on
> a competitive level, it seems desirable to follow the lead of the bigger
> project if users are ever expected to easily transition from Dozer.
>
> I may have missed something. Look forward to your thoughts.
>
> Cheers
> Ben
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys - and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> morph-developer mailing list
> mor...@li...
> https://lists.sourceforge.net/lists/listinfo/morph-developer
>
>
|
|
From: Ben A. <ben...@ac...> - 2007-01-20 01:17:38
|
Matt Sgarlata wrote:
> One situation I was considering is if you are doing something simple
> like Morph.convertToInteger("13asdf") you want a TransformationException
> instead of a JDK NumberFormatException. This would be in the interest
> of hiding implementation details of a particular transformer from a
> user. Fortunately, it appears the transformers happen to be written
> such that this is exactly what happens even with your change. Of course
> I will need to do testing to verify this is the case.
Hi Matt
If any transformer explicitly throws TransformationException, the patch
still propagates it up. That's probably why you're seeing the behavior
with convertToInteger (although not 100%, I haven't looked at the code).
All the patch does is modify all instances of "new
MorphException(Exception)" so that if the Exception is a
RuntimeException, throw RuntimeException directly. At no point does the
patch unwrap a TransformationException, opting instead to avoid creating
them in the first place.
Cheers
Ben
|
|
From: Ben A. <ben...@ac...> - 2007-01-23 05:08:21
|
Hi Matt Has there been any progress/decision on this issue? Cheers Ben |
|
From: Matt S. <Mat...@wh...> - 2007-01-23 15:45:15
|
I've been thinking, and the original intent of the code in BaseTransformer that wraps exceptions as TransformationExceptions was put in place for 2 reasons: 1) to ensure simple transformers bundled with Morph threw TransformationExceptions rather than the various exceptions thrown by Java, and 2) so that subclasses could throw checked exceptions if needed and have those changed to runtime exceptions Wrapping all exceptions for all subclasses of BaseTransformer was kind of a side effect I hadn't thought through. So this is a very long-winded way of saying that yes, I think we should make the change that runtime exceptions are not wrapped in TransformationExceptions. In terms of implementation, I think the easiest way to do this would be to introduce a new boolean flag that indicates whether or not the BaseTransformer should wrap TransformationExceptions as RuntimeExceptions. The default value would be false, meaning do not wrap RuntimeExceptions as TransformationExceptions. I think this issue impacts more than just Transformers. For the sake of consistency, we would want to change this behavior in Reflectors (I think your patch may have covered these), Languages, Contexts and perhaps Wrappers as well (I'm a little rusty myself on what the current behavior of Wrappers is). I'm very busy at work for the beginning of this week, but hopefully will have some time to look into this toward the end of this week. Matt Ben Alex wrote: > Hi Matt > > Has there been any progress/decision on this issue? > > Cheers > Ben > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys - and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > morph-developer mailing list > mor...@li... > https://lists.sourceforge.net/lists/listinfo/morph-developer > > |
