Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom ActionException subclasses are not rethrown as new ActionException #340

Merged
merged 4 commits into from
Nov 15, 2013

Conversation

spg
Copy link
Member

@spg spg commented Nov 13, 2013

This is a tentative fix for #312

Notice that the actual fix is in AbstractDispatchServiceImpl. All other classes in this patch are for testing purposes only.

I remove all the stacktrace from the thrown exception using e.setStackTrace(new StackTraceElement[]{});

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nl?

@branflake2267
Copy link
Contributor

Nice Job

@meriouma
Copy link
Member

Checkstyle is failing I think

@meriouma
Copy link
Member

Fix build, otherwise looks good!

@meriouma
Copy link
Member

I'll plan a patch release for the end of the week

throw new ActionException(e.getMessage());
e.setStackTrace(new StackTraceElement[]{});

throw e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure that e is logged before throwing it to the client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure causes (and their stack trace) are not propagated either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think logging is already done by logger.log(Level.WARNING, newMessage, e); no?

@spg
Copy link
Member Author

spg commented Nov 14, 2013

In 895a5bd I removed the cause's stacktrace. What do yo think of this @Chris-V @christiangoudreau ? I don't think we can remove the cause completely, since e.initCause will throw.

@@ -0,0 +1,25 @@
/**
* Copyright 2011 ArcBees Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright 2013

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvermind, what Christian said prolly work better. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update all copyrights of all all files to 2013 in another PR

On Thu, Nov 14, 2013 at 10:33 AM, Brandon Donnelson <
[email protected]> wrote:

In
gwtp-core/gwtp-dispatch-server-guice/src/test/java/com/gwtplatform/dispatch/server/ActionExceptionThrownByValidator.java:

@@ -0,0 +1,25 @@
+/**

  • * Copyright 2011 ArcBees Inc.

copyright 2013


Reply to this email directly or view it on GitHubhttps://github.com//pull/340/files#r7662155
.

Simon-Pierre Gingras | *Developer
M: 418.929.0230 *|
T: @spgingras | *G+: SP Gingras
ArcBees http://www.arcbees.com/ *|
Blog http://blog.arcbees.com/ |
Facebook http://facebook.com/QueenOfTheBees |
LinkedInhttp://www.linkedin.com/company/arcbees

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the open source copy right header, is that automatic?

@branflake2267
Copy link
Contributor

Nice job.

@spg
Copy link
Member Author

spg commented Nov 14, 2013

@Chris-V See 693dcca

@branflake2267
Copy link
Contributor

Would it be useful to add more javadoc?

@Chris-V
Copy link
Member

Chris-V commented Nov 14, 2013

lgtm, good job

@spg
Copy link
Member Author

spg commented Nov 14, 2013

@branflake2267 Do you see places where it would be useful?

@branflake2267
Copy link
Contributor

I would think Javadoc could be a bonus on any api classes the dev might want to touch. I'm of the opinion more is better with Javadoc. :)

@christiangoudreau
Copy link
Member

+1 to add information in the javadoc of the dispatcher that it erase the full stacktrace to avoid security holes.

@spg
Copy link
Member Author

spg commented Nov 14, 2013

Added a little bit of documentation in f51282c . Not sure if enough or correct (I'm not a big javadoc writer :P)

@christiangoudreau
Copy link
Member

LGTM

spg pushed a commit that referenced this pull request Nov 15, 2013
Custom ActionException subclasses are not rethrown as new ActionException
@spg spg merged commit 093029b into master Nov 15, 2013
spg pushed a commit that referenced this pull request Apr 4, 2014
Custom ActionException subclasses are not rethrown as new ActionException
hpehl pushed a commit to hpehl/GWTP that referenced this pull request Dec 9, 2014
Custom ActionException subclasses are not rethrown as new ActionException


Former-commit-id: 0fb839e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants