Add injection point for handling exceptions #742

archolewa · 2018-07-13T15:57:18Z

-- Before we hard-coded how we handled exceptions in the Servlets.
For example, by default we return a 400 - Bad Request if we get an
Exception. However, this is very inflexible. For example, suppose
customers have some customizations (like their own SearchProviders) that
may generate errors the DataServlet doesn't know to look for, but aren't
the fault of the user. Then, Fili currently returns a 400, even though
it should probably be some sort of 5xx.

So we add an injection point to all servlets that allows customers to
provide their own logic for handling DataServlet errors.

QubitPi

Change log needed

QubitPi · 2018-07-16T19:59:59Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/DataExceptionHandler.java

+
+/**
+ * Allows a customer to inject custom code for handling Exceptions in Fili's DataServlet.
+ *


Need a <p> tag to start the next paragraph

QubitPi · 2018-07-16T21:44:12Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/ExceptionHandler.java

+ * For example, customers may want to return 500's for some custom exceptions, or return
+ * an empty set when dimension rows are not found.
+ */
+public interface ExceptionHandler {


Where is this interface used?

This is vestigial, and should be removed.

QubitPi · 2018-07-16T21:45:38Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/DataExceptionHandler.java

+ * For example, customers may want to return 500's for some custom exceptions, or return
+ * an empty set when dimension rows are not found.
+ */
+public interface DataExceptionHandler {


Why do we make DataExceptionHandler and MetadataExceptionHandler separate?

Because of how the two servlets work. The DataServlet uses asynchronous responses, so we need to pass in the asyncResponse for this method to resume.

The metadata servlets meanwhile use synchronous responses (if you look at their get* methods, you'll see that they return a Response), so we need to do the same thing in the Metadata interface.

Kinda hard to put both of those behaviors into one interface.

michael-mclawhorn · 2018-07-16T21:48:56Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/ExceptionHandler.java

+ * For example, customers may want to return 500's for some custom exceptions, or return
+ * an empty set when dimension rows are not found.
+ */
+public interface ExceptionHandler {


This is orphaned?

michael-mclawhorn · 2018-07-16T21:50:16Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/MetadataExceptionHandler.java

+     */
+    Response handleThrowable(
+            Throwable e,
+            Optional<ApiRequest> request,


Should this be ? extends ApiRequest allowing the subclasses to be typesafer?

QubitPi

I should've commented instead of requesting changes. Please let @michael-mclawhorn take a look as well

QubitPi · 2018-07-16T22:34:25Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/MetadataExceptionHandler.java

+ * a result, there is not a clean way to unify them into a single exception handler. So they each
+ * get their own
+ */
+public interface MetadataExceptionHandler {


Maybe we can have a marker interface ExceptionHandler whose child interfaces are MetadataExceptionHandler and DataExceptionHandler

That sounds like unnecessary infrastructure to me. Since both interfaces
have such different signatures, we'll still need to either use the
(Meta)DataExceptionHandler in the appropriate Servlet, or we'll have to cast.
So I don't know what such a marker interface buys us.

QubitPi · 2018-07-16T22:35:26Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/FiliDataExceptionHandler.java

+
+/**
+ * The default implementation of {@link DataExceptionHandler}.
+ *


QubitPi · 2018-07-16T22:39:15Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/endpoints/DimensionsServlet.java

-            String msg = ErrorMessageFormat.REQUEST_PROCESSING_EXCEPTION.format(e.getMessage());
-            LOG.error(msg, e);
-            responseSender = () -> Response.status(Status.BAD_REQUEST).entity(msg).build();
+            return response;


👍 I used to have no idea what responseSender is for

michael-mclawhorn · 2018-07-16T23:24:47Z

fili-core/src/main/java/com/yahoo/bard/webservice/exception/FiliJobsExceptionHandler.java

@@ -41,10 +41,9 @@ public FiliJobsExceptionHandler(ObjectMappersSuite objectMappers) {
    @Override
    public Response handleThrowable(
            Throwable e,
-            Optional<ApiRequest> request,
+            Optional<? extends ApiRequest> request,


I was thinking that these would become more type specific. e.g. Optional etc.

I'm not sure what you're asking. So far as I know, you can't make a type more specific when implementing an interface method. Are you suggesting providing a separate interface for each metadata endpoint?

michael-mclawhorn · 2018-07-17T17:19:37Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/endpoints/DataServlet.java

+            );
+            // Generally, it's expected that implementations of `ExceptionHandler` will resume
+            // the response in every case. This exists so that if someone writes a buggy handler
+            // that fails to resume the response, they at least get *something* back.


The 'just in case' asynch response resume should check if the response is already done and if it /isn't/ send a message indicating that an error was mishandled. (A 500 error, probably, also passing along the error that was badly handled)

michael-mclawhorn

Restated below.

michael-mclawhorn

This thing may want for a quick test, maybe directly on dataservlet.getData(), initialize with a bunch of Mocks, throw an NPE early in processing the request, and then have an error handler for NPE catch and respond to it correctly.

Also, now that the error handlers exist externally, it would be very easy to write regression tests instantiating them and stimulating their various exception handling scenarios. It would also be a hint to people building their own to make tests on them.

archolewa · 2018-07-17T22:16:25Z

@michael-mclawhorn I'm hesitant to do a direct test on getData like that, because that will be a very brittle test. Every time somebody adds a new injection point, that test will break even if it has nothing to do error handling, because we'll need to build a DataServlet. Same if/when we add additional query parameters, and modify getData. Meanwhile, we already have the ErrorDataServletSpec which forces and tests a variety of error situations in a more stable fashion.

The only thing the ErrorServlet isn't going to test directly (though it does exercise) is the failsafe response if someone misses a step, so that's the only thing that could benefit from such a test.

I'm really hesitant to add a brittle test to test something that doesn't happen in the default implementation.

That being said, I have no problem adding unit tests for the exception handlers.

-- Before we hard-coded how we handled exceptions in the Servlets. For example, by default we return a 400 - Bad Request if we get an Exception. However, this is very inflexible. For example, suppose customers have some customizations (like their own SearchProviders) that may generate errors the DataServlet doesn't know to look for, but aren't the fault of the user. Then, Fili currently returns a 400, even though it should probably be some sort of 5xx. So we add an injection point to all servlets that allows customers to provide their own logic for handling DataServlet errors.

-- Also, removes the unnecessary `metadataEntityName` parameter from the `MetadataExceptionHandler`. That was used to pass along the dimension name for one of the error messages in the `DimensionsServlet`, but the `ApiRequest` contains the dimension name already.

-- Use `DataApiRequest` rather than `? extends ApiRequest` in `DataExceptionHandler`. -- Improves some Javadoc.

-- Tests have been added for unit tests, but not the direct test on `DataServlet` that Michael recommended because of how brittle such a test would be.

archolewa · 2018-07-18T19:45:18Z

Added some tests, as requested by @michael-mclawhorn. Note that I very explicitly did not try to merge the tests under a shared specification, despite them having some shared tests. The implementations are all completely separate, and I think the shared implementation are a coincidence, and not because of any inherent relationship. Also, these days I like keeping my tests self-contained (navigating Specification trees is annoying) if I can, and the shared tests are simple enough that I don't think we got a whole lot out of trying to share them anyway.

archolewa force-pushed the customizeException branch 2 times, most recently from ae5b873 to 279a162 Compare July 13, 2018 17:01

yahoo deleted a comment Jul 13, 2018

QubitPi added the REVIEWABLE label Jul 13, 2018

QubitPi suggested changes Jul 16, 2018

View reviewed changes

QubitPi added the NEED REBASE label Jul 16, 2018

michael-mclawhorn reviewed Jul 16, 2018

View reviewed changes

QubitPi approved these changes Jul 16, 2018

View reviewed changes

QubitPi added NEED 1 REVIEW and removed REVIEWABLE labels Jul 16, 2018

michael-mclawhorn reviewed Jul 16, 2018

View reviewed changes

yahoo deleted a comment Jul 17, 2018

michael-mclawhorn reviewed Jul 17, 2018

View reviewed changes

michael-mclawhorn requested changes Jul 17, 2018

View reviewed changes

yahoo deleted a comment Jul 17, 2018

yahoo deleted a comment Jul 18, 2018

Andrew Cholewa added 3 commits July 18, 2018 14:42

Address comments.

8287ecc

-- Also, removes the unnecessary `metadataEntityName` parameter from the `MetadataExceptionHandler`. That was used to pass along the dimension name for one of the error messages in the `DimensionsServlet`, but the `ApiRequest` contains the dimension name already.

Address comments.

bc9a731

-- Use `DataApiRequest` rather than `? extends ApiRequest` in `DataExceptionHandler`. -- Improves some Javadoc.

Andrew Cholewa added 2 commits July 18, 2018 14:42

Address comment.

02fdcb2

Adds tests.

cf978a4

-- Tests have been added for unit tests, but not the direct test on `DataServlet` that Michael recommended because of how brittle such a test would be.

archolewa force-pushed the customizeException branch from f6e0efd to cf978a4 Compare July 18, 2018 19:43

yahoo deleted a comment Jul 18, 2018

michael-mclawhorn approved these changes Jul 18, 2018

View reviewed changes

QubitPi added MERGEABLE and removed NEED 1 REVIEW NEED REBASE labels Jul 18, 2018

archolewa merged commit 2173d9a into master Jul 18, 2018

archolewa deleted the customizeException branch July 18, 2018 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add injection point for handling exceptions #742

Add injection point for handling exceptions #742

archolewa commented Jul 13, 2018

QubitPi left a comment

QubitPi Jul 16, 2018

QubitPi Jul 16, 2018

archolewa Jul 16, 2018

QubitPi Jul 16, 2018

archolewa Jul 16, 2018

michael-mclawhorn Jul 16, 2018

michael-mclawhorn Jul 16, 2018

QubitPi left a comment

QubitPi Jul 16, 2018

archolewa Jul 17, 2018

QubitPi Jul 16, 2018

QubitPi Jul 16, 2018

michael-mclawhorn Jul 16, 2018

archolewa Jul 17, 2018

michael-mclawhorn Jul 17, 2018

michael-mclawhorn left a comment •

edited

Loading

michael-mclawhorn left a comment •

edited

Loading

archolewa commented Jul 17, 2018

archolewa commented Jul 18, 2018

Add injection point for handling exceptions #742

Add injection point for handling exceptions #742

Conversation

archolewa commented Jul 13, 2018

QubitPi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-mclawhorn left a comment • edited Loading

Choose a reason for hiding this comment

michael-mclawhorn left a comment • edited Loading

Choose a reason for hiding this comment

archolewa commented Jul 17, 2018

archolewa commented Jul 18, 2018

michael-mclawhorn left a comment •

edited

Loading

michael-mclawhorn left a comment •

edited

Loading