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

#859 Add error-handling best practice in quickstart-se #860

Merged
merged 16 commits into from
Sep 7, 2019
Merged

#859 Add error-handling best practice in quickstart-se #860

merged 16 commits into from
Sep 7, 2019

Conversation

camerondurham
Copy link
Contributor

@camerondurham camerondurham commented Jul 26, 2019

Closes my issue #859 in examples/quickstarts/helidon-quickstart-se

@camerondurham camerondurham changed the title Fixed put request vulnerability in quickstart-se and Maven build error #859 Fixed put request vulnerability in quickstart-se and Maven build error Jul 26, 2019
@camerondurham camerondurham changed the title #859 Fixed put request vulnerability in quickstart-se and Maven build error #859 Fixed lagging PUT request in quickstart-se and Maven build error Jul 26, 2019
@romain-grecourt
Copy link
Contributor

Hi @camerondurham, the version change part of your PR is not valid.
I'll let other people chime in regarding the exception handling change.

The examples in the master branch depend on the current SNAPSHOT version and thus require a full build.
If you wish to try-out examples for a given release, you can follow the instructions documented here: https://github.com/oracle/helidon/blob/master/examples/README.md

@camerondurham
Copy link
Contributor Author

HI @romain-grecourt , thank you for the update. Should I instead remove this request and make only the .exceptionally(...) fix?

@romain-grecourt
Copy link
Contributor

Yes, go ahead.

@camerondurham camerondurham changed the title #859 Fixed lagging PUT request in quickstart-se and Maven build error #859 Add error-handling best practice in quickstart-se Aug 1, 2019
@camerondurham
Copy link
Contributor Author

@romain-grecourt My PR now only updates the quickstart.

@romain-grecourt
Copy link
Contributor

romain-grecourt commented Aug 7, 2019

There are a couple of issues with your suggested change.

  1. If you submit invalid JSON, it would also return a 400 with "empty body"
  2. A good practice would be to show the use of request.next(throwable) to delegate the error to error handlers.

Here is what I came up with:

        request.content().as(JsonObject.class)
                .thenAccept(jo -> updateGreetingFromJson(jo, response))
                .exceptionally((Throwable ex) -> {
                    if (ex.getCause() instanceof JsonException) {
                        request.next(new BadRequestException("Invalid JSON",
                                (JsonException) ex.getCause()));
                    } else {
                        request.next(ex);
                    }
                    return null;
                });

@tomas-langer thoughts ?

@camerondurham
Copy link
Contributor Author

Thanks for pointing out those issues. Of course, I agree that your change is much more effective. Sorry for being unfamiliar with this process, but would you be making this change yourself or should I modify my PR? @romain-grecourt

@romain-grecourt
Copy link
Contributor

Ideally you'd make this change in your PR so that you could get a Helidon contrib under your Belt.

@romain-grecourt
Copy link
Contributor

@barchetta @tomas-langer Any thoughts on the suggested changes ?

@camerondurham
Copy link
Contributor Author

Thanks, that sounds great. I'm adding @namanshenoy to my fork since he's another Oracle employee I worked with on this.

@romain-grecourt
Copy link
Contributor

@tomas-langer What do you think about returning the exception message in the response body ?

Copy link
Contributor

@romain-grecourt romain-grecourt left a comment

Choose a reason for hiding this comment

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

Sorry, I approved by mistake ; the error handling needs to be refined.

Copy link
Contributor

@romain-grecourt romain-grecourt left a comment

Choose a reason for hiding this comment

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

LGTM, @tomas-langer @barchetta can you guys take a look ?

Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants