Custom Response creation part1 #447

tarrantzhang · 2017-07-26T18:59:11Z

This PR implements step 6,7 in RFC for custom response creation

Make httpResponseMaker injectable.
Change signature of httpResponseChannel constructor to take ApiRequest.

tarrantzhang · 2017-07-26T20:08:02Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseMaker.java

     *
     * @return Completely built response with headers and result set
     */
    public javax.ws.rs.core.Response buildResponse(
            PreResponse preResponse,
            ResponseFormatType responseFormatType,
-            UriInfo uriInfo
+            UriInfo uriInfo,
+            ApiRequest apiRequest


Not so sure if UriInfo and ResponseFormatType still needed as we can get them from ApiRequest.

I think we can safely get rid of the UriInfo and ResponseFormatType fields, and let people pull them from the ApiRequest when needed. I prefer to have one source of truth whenever possible.

The same applies to HttpResponseChannel.

QubitPi

CHANGELOG needed

QubitPi · 2017-07-26T22:21:07Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

@@ -907,6 +910,15 @@ protected JobPayloadBuilder buildJobPayloadBuilder() {
    }

    /**
+     * Initializes the factory that builds HttpResponseMaker.
+     *
+     * @return An instance of the {@link HttpResponseMaker}


Lowercase An (This is fili doc standard)

QubitPi · 2017-07-26T22:21:44Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseChannel.java

 import rx.Observer;

 import javax.ws.rs.container.AsyncResponse;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;

+import static com.yahoo.bard.webservice.web.handlers.workflow.DruidWorkflow.RESPONSE_WORKFLOW_TIMER;


static import goes to the top

QubitPi · 2017-07-26T22:22:28Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/HttpResponseMakerSpec.groovy

-import javax.ws.rs.core.PathSegment
-import javax.ws.rs.core.Response
-import javax.ws.rs.core.UriInfo
+import javax.ws.rs.core.*


Avoid start import

QubitPi · 2017-07-26T22:22:39Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/HttpResponseMakerSpec.groovy

+
+import static com.yahoo.bard.webservice.data.time.DefaultTimeGrain.DAY
+import static com.yahoo.bard.webservice.web.responseprocessors.ResponseContextKeys.API_METRIC_COLUMN_NAMES
+import static com.yahoo.bard.webservice.web.responseprocessors.ResponseContextKeys.REQUESTED_API_DIMENSION_FIELDS


static import goes to the top

archolewa

You missed a key step here: Passing in the HttpResponseMaker in as a constructor argument to DataServlet. If you don't do that, then Fili won't actually use the HttpResponseMaker bound in the AbstractBinderFactory!

archolewa · 2017-07-27T18:56:48Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseMaker.java

     *
     * @return Completely built response with headers and result set
     */
    public javax.ws.rs.core.Response buildResponse(
            PreResponse preResponse,
            ResponseFormatType responseFormatType,
-            UriInfo uriInfo
+            UriInfo uriInfo,
+            ApiRequest apiRequest


I think we can safely get rid of the UriInfo and ResponseFormatType fields, and let people pull them from the ApiRequest when needed. I prefer to have one source of truth whenever possible.

The same applies to HttpResponseChannel.

archolewa · 2017-07-27T19:00:16Z

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

@@ -404,6 +405,7 @@ public void getData(
                    apiRequest.getAsyncAfter(),
                    apiRequest.getFormat(),
                    uriInfo,
+                    apiRequest,


Since we have to pass the entire apiRequest into this method anyway, we might as well drop the parameters that we pull directly from the apiRequest (i.e. asyncAfter, format, the uriInfo). It'll make this method's signature a little bit smaller, which is always nice when you can get away with it.

I don't really have a good place to put this, because github stupidly insists on limiting what lines I can comment on. Right now, we're not actually using the HttpResponseMaker that is being injected. You'll note that on line 397 we are still manually building the HttpResponseMaker. So it doesn't matter what customers put in the AbstractBinderFactory, we'll still build and use the version defined in Fili!

So, In addition to providing an injection point in the AbstractBinderFactory, you also need to make the HttpResponseMaker a constructor argument for DataServlet.

Since DataServlet has the @Inject annotation, HK2 will build it for us, and as a part of that, it will build all of the constructor arguments and pass them in to the DataServlet.

archolewa · 2017-07-27T19:06:07Z

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

-                                httpResponseMaker,
-                                apiRequest.getFormat(),
-                                apiRequest.getUriInfo()
+                                apiRequest,


We also need to inject the HttpResponseMaker into the JobsServlet

archolewa

Just the one comment that can easily be handled in the next PR.

archolewa · 2017-07-28T15:29:52Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseMaker.java

    ) {
        ResponseBuilder rspBuilder = createResponseBuilder(
                preResponse.getResultSet(),
                preResponse.getResponseContext(),
-                responseFormatType,
-                uriInfo
+                apiRequest.getFormat(),


The response builder should also take the full ApiRequest instead of just the format and uriInfo, or are you planning to make that change in the next PR as part of splitting Response? If so, feel free to disregard this comment.

QubitPi

I didn't see any implementation for step 5. Is that intended?

QubitPi · 2017-07-28T17:30:01Z

CHANGELOG.md

+- [Make HttpResponseMaker injectable](https://github.com/yahoo/fili/pull/447)
+    * Make `HttpResponseMaker` injectable. `DataServlet` and `JobsServlet` takes `HttpResponseMaker` as input parameter now
+
+- [Change functions signature related to custom response creation](https://github.com/yahoo/fili/pull/447)


One entry per PR

QubitPi · 2017-07-28T17:46:56Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseMaker.java

    ) {
        @SuppressWarnings("unchecked")
+        ResponseFormatType responseFormatType = apiRequest.getFormat();
+        UriInfo uriInfo = apiRequest.getUriInfo();


uriInfo is used only once. It's better to have ResponseFormat.getCsvContentDispositionValue(apiRequest.getUriInfo()) instead of ResponseFormat.getCsvContentDispositionValue(uriInfo)

QubitPi · 2017-07-28T17:47:37Z

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

@@ -114,6 +117,8 @@
    private final ObjectWriter writer;
    private final ObjectMappersSuite objectMappers;



remove empty line

QubitPi · 2017-07-28T17:49:37Z

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

@@ -474,15 +470,15 @@ private void setupAsynchronousWorkflows(
        // resources once per subscription. A connectable Observable's chain is only executed once
        // regardless of the number of subscriptions.
        ConnectableObservable<Either<PreResponse, JobRow>> payloadEmitter;
-        if (asyncAfter == DataApiRequest.ASYNCHRONOUS_ASYNC_AFTER_VALUE) {
+        if (apiRequest.getAsyncAfter() == DataApiRequest.ASYNCHRONOUS_ASYNC_AFTER_VALUE) {


Pull it into a variable, since it's used multiple times

QubitPi · 2017-07-28T17:49:52Z

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

            Observable<PreResponse> queryResultsEmitter,
            ContainerRequestContext containerRequestContext,
            AsyncResponse asyncResponse,
            HttpResponseMaker httpResponseMaker
    ) {
-        JobRow jobMetadata = jobRowBuilder.buildJobRow(uriInfo, containerRequestContext);
+        JobRow jobMetadata = jobRowBuilder.buildJobRow(apiRequest.getUriInfo(), containerRequestContext);


Pull it into a variable, since it's used multiple times

QubitPi · 2017-07-28T18:02:36Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

@@ -302,6 +303,8 @@ protected void configure() {

                bind(getClock()).to(Clock.class);

+                bind(getHttpResponseMaker()).to(HttpResponseMaker.class);


This needs a test. At least testing that it's been binded like 1 * dc.bind({ it.implementation.contains(HttpResponseMaker.canonicalName) }, _) in "test configure bindings"()

I disagree. This is a dirt-simple binding. All we'd be testing is the HK2 injection framework. Far as I can tell we test hardly any of the other similarly simple bindings (i.e. we don't verify that the ObjectMapperSuite is being bound correctly.

QubitPi · 2017-07-28T18:11:33Z

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

-     * @param asyncAfter  How long the user is willing to wait for a synchronous request
-     * @param responseFormat  The requested format for the response
-     * @param uriInfo  The URI of the request
+     * @param apiRequest The apiRequest


Param description doesn't help too much. People already know that from param name. Descriptions talks about what it is and how it's gonna be used

QubitPi · 2017-07-28T18:18:12Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseMaker.java

@@ -61,21 +64,18 @@ public HttpResponseMaker(ObjectMappersSuite objectMappers, DimensionDictionary d
     * Build complete response.
     *
     * @param preResponse  PreResponse object which contains result set, response context and headers
-     * @param responseFormatType  The format in which the response should be returned to the user
-     * @param uriInfo  UriInfo of the request
+     * @param apiRequest apiRequest needed for serialization writer


2 spaces between param name and param desc. Capitalize first word in description (This is fili doc standard) ¯_(ツ)_/¯

QubitPi · 2017-07-28T18:18:47Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseMaker.java

@@ -95,18 +95,18 @@ public HttpResponseMaker(ObjectMappersSuite objectMappers, DimensionDictionary d
     *
     * @param resultSet  The result set being processed
     * @param responseContext  A meta data container for the state gathered by the web container
-     * @param responseFormatType  The format in which the response should be returned to the user
-     * @param uriInfo  UriInfo of the request
+     * @param apiRequest apiRequest needed for serialization writer


2 spaces between param name and param desc. Capitalize first word in description

QubitPi · 2017-07-28T18:45:54Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/HttpResponseChannel.java

@@ -58,16 +35,15 @@ public HttpResponseChannel(
     * @param apiRequest  Api request object with all the associated info with it
     * @param httpResponseMaker  Helper class instance to prepare the response object
     *
-     * @deprecated  The ResponseFormatType and UriInfo should be passed explicitly, rather than implicitly via the


@michael-mclawhorn @archolewa Is it really necessary to pass the giant ApiRequest object? It de-separate layer abstraction and makes it more difficult to set up a test. Does passing more variables from ApiRequest solves the problem, instead of passing ApiRequest?

The purpose of making the HttpResponseChannel injectable is so that customers can inject their own HttpResponseChannel so that they can customize how they send back the response.

There are situations (like the one that motivated this change) where customers will want to customize the response format based on something in the request. However, we can't very well predict what components of the request customers will use to determine how to send the response.

And it actually doesn't it make that much harder to setup a test since Groovy ignores access modifiers and the DataApiRequest (at least) has a private default constructor. So you can build an empty ApiRequest and then use the request's withers to initialize only the pieces you care about for the test.

archolewa · 2017-07-28T19:06:20Z

@QubitPi The steps are mislabeled in the RFC (there are two 5's). He's implementing the second five

tarrantzhang · 2017-07-28T19:51:24Z

@QubitPi Sorry didn't sync up the PR description with RFC, it should be steps 6 & 7.

tarrantzhang commented Jul 26, 2017

View reviewed changes

QubitPi added NEED REBASE NEED 2 REVIEWS REVIEWABLE labels Jul 26, 2017

QubitPi suggested changes Jul 26, 2017

View reviewed changes

archolewa requested changes Jul 27, 2017

View reviewed changes

archolewa added the NEED CHANGES label Jul 27, 2017

yahoo deleted a comment Jul 27, 2017

archolewa approved these changes Jul 28, 2017

View reviewed changes

archolewa added NEED 1 REVIEW NEED CHANGES and removed NEED 2 REVIEWS NEED CHANGES labels Jul 28, 2017

yahoo deleted a comment Jul 28, 2017

QubitPi removed the NEED REBASE label Jul 28, 2017

QubitPi suggested changes Jul 28, 2017

View reviewed changes

yahoo deleted a comment Jul 28, 2017

QubitPi approved these changes Jul 28, 2017

View reviewed changes

QubitPi added NEED REBASE and removed NEED 1 REVIEW NEED CHANGES REVIEWABLE labels Jul 28, 2017

This was referenced Jul 28, 2017

Custom Response creation part2 tarrantzhang/fili#1

Open

Custom Response creation part2 #455

Merged

yahoo deleted a comment Jul 28, 2017

refactor HttpResponseMaker and make it injectable

2082607

tarrantzhang force-pushed the refactor-httpresponsemaker branch from 719fc1f to 2082607 Compare July 28, 2017 21:24

yahoo deleted a comment Jul 28, 2017

QubitPi added MERGEABLE and removed NEED REBASE labels Jul 28, 2017

QubitPi merged commit ab4929f into yahoo:master Jul 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Response creation part1 #447

Custom Response creation part1 #447

tarrantzhang commented Jul 26, 2017 •

edited

tarrantzhang Jul 26, 2017

archolewa Jul 27, 2017

QubitPi left a comment

QubitPi Jul 26, 2017 •

edited

QubitPi Jul 26, 2017

QubitPi Jul 26, 2017

QubitPi Jul 26, 2017

archolewa left a comment

archolewa Jul 27, 2017

archolewa Jul 27, 2017

archolewa Jul 27, 2017

archolewa Jul 27, 2017

archolewa left a comment

archolewa Jul 28, 2017

QubitPi left a comment

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017

archolewa Jul 28, 2017

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017

QubitPi Jul 28, 2017 •

edited

archolewa Jul 28, 2017 •

edited

archolewa Jul 28, 2017

archolewa commented Jul 28, 2017 •

edited

tarrantzhang commented Jul 28, 2017

		@@ -114,6 +117,8 @@
		private final ObjectWriter writer;
		private final ObjectMappersSuite objectMappers;

		@@ -302,6 +303,8 @@ protected void configure() {

		bind(getClock()).to(Clock.class);

		bind(getHttpResponseMaker()).to(HttpResponseMaker.class);

Custom Response creation part1 #447

Custom Response creation part1 #447

Conversation

tarrantzhang commented Jul 26, 2017 • edited

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

QubitPi Jul 26, 2017 • edited

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

archolewa 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

archolewa left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi Jul 28, 2017 • edited

Choose a reason for hiding this comment

archolewa Jul 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archolewa commented Jul 28, 2017 • edited

tarrantzhang commented Jul 28, 2017

tarrantzhang commented Jul 26, 2017 •

edited

QubitPi Jul 26, 2017 •

edited

QubitPi Jul 28, 2017 •

edited

archolewa Jul 28, 2017 •

edited

archolewa commented Jul 28, 2017 •

edited