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

Dont swallow exceptions in binder #987

Merged

Conversation

asolntsev
Copy link
Contributor

See ticket 2032

@asolntsev
Copy link
Contributor Author

Finally, I have modified Binder.java so that instead of old code

        } catch (Exception e) {
            Validation.addError(paramNode.getOriginalKey(), "validation.invalid");
        }

now works the tuned code:

} catch (NumberFormatException e) {
    logBindingFailure(paramNode, e);
    Validation.addError(paramNode.getOriginalKey(), "validation.invalid");
} catch (ParseException e) {
    logBindingFailure(paramNode, e);
    Validation.addError(paramNode.getOriginalKey(), "validation.invalid");
} catch (Exception e) {
    // This is bad catch. I would like to remove it in next version.
    Logger.error(e, "Failed to bind %s=%s", paramNode.getOriginalKey(), Arrays.toString(paramNode.getValues()));
    Validation.addError(paramNode.getOriginalKey(), "validation.invalid");
}

@asolntsev asolntsev added this to the 1.4.3 milestone Jul 2, 2016
@asolntsev asolntsev self-assigned this Jul 2, 2016
Binders can only throw ParseException and NumberFormatException
@asolntsev asolntsev force-pushed the dont-swallow-exceptions-in-binder branch from b31d3be to bc4a2f6 Compare July 2, 2016 22:24
@asolntsev
Copy link
Contributor Author

NB! If you have custom Binders in your code, you need to slightly change the signature.

Instead of

public T bind() throws Exception {

you need to declare

public T bind() throws ParseException {

@asolntsev asolntsev merged commit 3b40c51 into playframework:1.4.x Jul 2, 2016
@asolntsev asolntsev deleted the dont-swallow-exceptions-in-binder branch July 2, 2016 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant