[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: |
|
| 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/ 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. |