[MSHOP-130] DefaultCustomDataAccesor#getNodeByName should not throw java.lang.Exception but a subclass Created: 15/Mar/14  Updated: 16/Mar/23  Resolved: 16/Mar/23

Status: Closed
Project: Magnolia Shop (closed)
Component/s: None
Affects Version/s: 1.1.4
Fix Version/s: 1.1.5, 2.0.1

Type: Improvement Priority: Minor
Reporter: Edgar Vonk Assignee: Unassigned
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: PNG File Screen Shot 2014-03-15 at 17.45.08.png    
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Description   

The problem with this is that we end up having to catch java.lang.Exception all over our code. Simply because this method in the Magnolia Shop throws it. And therefore all methods that use it (quite a lot).

I think it is bad practice to throw java.lang.Exception. You should always throw a specific subclass so that you (and we) can implement good exception handling.

In general I think the exception handling in the shop module could be improved here and there but this is the most annoying one for us I guess.



 Comments   
Comment by Edgar Vonk [ 15/Mar/14 ]

Ah, some other exception handling issues in the shop module come to mind..

1/ ShopConfigurationException does not include a constructor for specifying an root cause exception. this is also bad practice I think. please simply add:

public ShopConfigurationException(String string, Throwable cause) {
        super(string, cause);
    }

2/
This results in code in the shop module that does this (evil!):

catch (Exception e) {
            throw new ShopConfigurationException("Unable to instantiate cart class");
        }

Root cause exceptions should always be passed on when you rethrow them. This makes it extremely hard, if not impossible, to find the real causes of problems when using the shop module.

The most problematic one that we have run into is in SaveAndConfirmProcessor#internalProcess. When for some reason the shopping cart cannot be saved this is only throws as a FormProcessorFailedException with a text message. The original root cause is lost and is also not logged. So when this happens on a production system (like it did in our case) there is absolutely no way to know what went wrong..

catch (Exception e) {
        //initialize new cart
        MgnlContext.getWebContext().getRequest().getSession().removeAttribute(ShopUtil.ATTRIBUTE_SHOPPINGCART);
        ShopUtil.setShoppingCartInSession();
        throw new FormProcessorFailedException("Error while proccessing your shopping cart");
    }
Comment by Adam Jones [ 16/Mar/23 ]

Closing due to project being archived.

Generated at Mon Feb 12 07:09:36 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.