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

Fix too wider IllegalArgumentException catch #2593

Merged
merged 8 commits into from Jul 25, 2019

Conversation

@bigwheel
Copy link
Contributor

commented Jul 5, 2019

  • Have you read through the contributor guidelines?
  • Have you signed the Lightbend CLA?
  • Have you updated the documentation?
    • no need to fix doc
  • Have you added tests for any changed functionality?
    -->

Purpose

Fix inconsistent behavior.

References

Nothing. This PR includes issue report, proof test and fix patch all.

Changes

If there are both Case Class Extraction and throw new IllegalArgumentException in single routing, response becomes 500 Internal Server Error from 400 Bad Request 400 Bad Request from 500 Internal Server Error.

Background Context

When throw IllegalArgumentException in routing, normally it results 500 internal server error.
However, if there is Case Class Extraction it results 400 bad request because of too wider catch.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@raboof

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

OK TO TEST

@akka-ci akka-ci added the validating label Jul 5, 2019
Copy link
Member

left a comment

Good observation. I agree it is inconsistent that throw new IllegalArgumentException has a different effect depending on whether there is an extraction around it or not.

I'm not sure whether we should prefer a 400 or a 500 here - but I like how self-contained this change is, so that might make it preferable.

try inner(tupler(f(values)))(ctx)
catch {
case e: IllegalArgumentException => ctx.reject(ValidationRejection(e.getMessage.nullAsEmpty, Some(e)))
Try(f(values)) match {

This comment has been minimized.

Copy link
@raboof

raboof Jul 5, 2019

Member

perhaps try/catch is a little more efficient since it saves some allocations?

This comment has been minimized.

Copy link
@bigwheel

bigwheel Jul 5, 2019

Author Contributor

I tried first try/catch way. But I couldn't work out code with try/catch because try/catch have not "not throwed case" (like ruby's else of "begin - rescue - else").

If you have good idea, I'm very happy :)

This comment has been minimized.

Copy link
@raboof

raboof Jul 5, 2019

Member

Hmm, you can break this out to a separate function and use 'return' in the 'catch' block... but I agree that's a bit weird perhaps this is OK then :)

This comment has been minimized.

Copy link
@bigwheel

bigwheel Jul 5, 2019

Author Contributor
try {
    val r = f(values)
    inner(tupler(r))(ctx) //
} catch {
    case e: IllegalArgumentException => ctx.reject(ValidationRejection(e.getMessage.nullAsEmpty, Some(e)))
}

This code doesn't solve the problem because ☆ code is stiil inside of try/catch.

val rOrRejected = try {
    f(values)
} catch {
    case e: IllegalArgumentException => ctx.reject(ValidationRejection(e.getMessage.nullAsEmpty, Some(e))) //
}

inner(tupler(r))(ctx) // XD  r is isntace of R or Future[RouteResult], result of △

But if I put ☆ outside of try-catch, I have to handle concrete r case and Future[RouteResult].

This comment has been minimized.

Copy link
@bigwheel

bigwheel Jul 5, 2019

Author Contributor

Hmm, you can break this out to a separate function and use 'return' in the 'catch' block... but I agree that's a bit weird perhaps this is OK then :)

Great Idea! Wait a moment ~

@akka-ci akka-ci added tested and removed validating labels Jul 5, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

Test PASSed.

bigwheel added 2 commits Jul 5, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

Test PASSed.

@raboof
raboof approved these changes Jul 9, 2019
Copy link
Member

left a comment

LGTM, it's probably only consistent to treat IllegalArgumentException special only for special places.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

Test FAILed.

Akka Jenkins
@lightbend-cla-validator

This comment has been minimized.

Copy link

commented Jul 25, 2019

@akka-ci akka-ci removed the validating label Jul 25, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

Test FAILed.

@lightbend-cla-validator

This comment has been minimized.

Copy link

commented Jul 25, 2019

@jrudolph

This comment has been minimized.

Copy link
Member

commented on 451f6a0 Jul 25, 2019

Good catch!

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

Test PASSed.

@jrudolph jrudolph merged commit 06cf14b into akka:master Jul 25, 2019
2 of 4 checks passed
2 of 4 checks passed
typesafe-cla-validator At least one pull request committer is not linked to a user.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Jenkins PR Auto-Formatter Successful
Details
Jenkins PR Validation Test PASSed. 4156 tests run, 1074 skipped, 0 failed.
Details
@jrudolph

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

(CLA check failed only because of autoformatter).

@jrudolph jrudolph added this to the 10.1.10 milestone Jul 25, 2019
@jrudolph

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Thanks a lot, @bigwheel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.