Custom Response creation part2 #455

tarrantzhang · 2017-07-28T20:43:21Z

Continuing on RFC for custom response creation.
Part 1.

Implementing steps 1~5:

Define interface ResponseWriter and its default implementation
Refactor Response class, splitting into ResponseData and three implementations of ResponseWriter
Define interface ResponseWriterSelector and its default implementation

Implement

    void addResponseType(ResponseType type, ResponseWriter writer);

Hook up the new serialization logic with HttpResponseMaker to replace the old one

tarrantzhang · 2017-07-31T16:36:04Z

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

@@ -88,7 +87,8 @@ public Response(
            SimplifiedIntervalList volatileIntervals,
            Map<String, URI> paginationLinks,
            Pagination pagination,
-            ObjectMappersSuite objectMappers
+            ObjectMappersSuite objectMappers,
+            ApiRequest apiRequest


I really hesitate to add apiRequest to ResponseData, but it's needed by write function in line 183

We should just remove that function. It no longer belongs on this class, and this is so deep into the guts of Fili, that I don't think we have to worry about removing it breaking anybody.

Basically, anything related to creating JSON or CSV no longer belongs on this class, and should be removed. That includes the ObjectMappers and the jsonFactory and csvMapper.

We can probably also drop the ResponseFormatType parameter, since the writers are pulling that off the ApiRequest.

archolewa

This still needs a little bit of work. That being said, the writers look pretty good, and I'm very happy about your efforts to document everything.

Mostly, we just need to refactor the tests to better line up with the new response format, finish tearing all the response writing out of ResponseData and we need to allow the user to inject a ResponseWriter, and then use it.

archolewa · 2017-07-31T19:01:23Z

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

+     *
+     * @throws IOException if a problem is encountered writing to the OutputStreamC
+     */
+    public void write(


Should be annotated with @Override

Also, since this method isn't changing the contract of the method it's implementing, JavaDoc isn't necessary.

archolewa · 2017-07-31T19:01:56Z

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

+     *
+     * @throws IOException if a problem is encountered writing to the OutputStreamC
+     */
+    public void write(


Annotated with @Override.

Also, since this method isn't changing the contract of the method it's implementing, JavaDoc isn't necessary.

archolewa · 2017-07-31T19:02:36Z

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

+            OutputStream os
+    ) throws IOException {
+        ResponseWriter writer = responseWriterSelector.select(request);
+        writer.write(request, responseData, os);


We should do a null check on writer here and throw a useful error message if the response type isn't in the map (i.e. isn't recognized). Otherwise, if at some point down the line, someone adds a custom response format, but forgets to register the writer, then they'll just get a vague NullPointerException.

archolewa · 2017-07-31T19:05:34Z

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

+     * @param request  ApiRequest object with all the associated info in it
+     * @return Response writer for the given format type
+     */
+    public ResponseWriter select(ApiRequest request) {


Should be annotated with @Override.

archolewa · 2017-07-31T19:07:24Z

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

+import java.util.Map;
+
+/**
+ * Response writer selector object.


This is the place where I would specify that this selector selects the ResponseWriter based on the format type in the ApiRequest, rather than on the constructor or method signature.

archolewa · 2017-07-31T19:26:18Z

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

+import java.io.OutputStream;
+
+/**
+ * Response writer interface.


We should provide a better class level JavaDoc. Nothing here says anything that the class declaration doesn't already.

In general, when writing JavaDoc you should focus on the what, and possibly the why depending on context (and how much tolerance your co-developers have for large class docs 😉 .

For example, I might write something like the following:

The interface for objects that write fully-processed ResultSets back to the user. This allows customers to fully customize how they choose to serialize the results from Fili based on the request.

archolewa · 2017-07-31T19:26:55Z

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

+     * @param os  OutputStream
+     * @throws IOException if a problem is encountered writing to the OutputStream
+     */
+    default void write(ApiRequest request, ResponseData responseData, OutputStream os)throws IOException {


Missing a space: OuputStream os)throws

archolewa · 2017-07-31T19:32:25Z

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

+
+
+/**
+ * Response writer selector object.


Again, this should provide a bit more insight into what this exists, and why it should be used. Maybe something like:

Typically, a Fili program's response writing infrastructure will consist of a collection of unrelated ResponseWriters each responsible for serializing the ResultSet into a specific format, and a single ResponseWriter that chooses the correct one based on the current state of the DataApiRequest. This interface allows customers provide a clean interface to the logic that makes that selection. Its sole purpose is to take a `DataApiRequest` and return the `ResponseWriter` that should be used to write the response.

archolewa · 2017-07-31T19:39:17Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/ResponseSpec.groovy

@@ -107,7 +107,11 @@ class ResponseSpec extends Specification {

    Set<Column> columns
    Set<LogicalMetric> testLogicalMetrics
-    Response response
+    ResponseData response


At a minimum, we should turn this into the FiliResponseWriterSpec, since that's what this is testing now.

However, we really should break this apart. Anything that tests only one format (i.e. just CSV, or just JSON) should be moved into a spec for the appropriate ResponseWriter. Anything that tests the getters in ResponseData should be moved into ResponseDataSpec.

In my mind, testing should be the following:

There should be dedicated Spec files for each writer, which test that they serialize the format correclty.

There should be a dedicated Spec that tests those ResponseData utility methods that are interesting.

There should be a Spec for FiliResponseWriter that ensures it selects the right writer (heh).

archolewa · 2017-07-31T19:44:52Z

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

@@ -136,7 +136,7 @@ private ResponseBuilder createResponseBuilder(
                                LinkedHashMap::new
                        ));

-        Response response = new Response(
+        ResponseData response = new ResponseData(


You're building a ResponseData object, but you're not actually using a ResponseWriter anywhere (except through the ResponseData::write method that should be removed). In order to be effective, the ResponseWriter needs to be injected, just like the HttpResponseMaker:

Add a binding of ResponseWriter to the AbstractBinderFactory that defaults to the FiliResponseWriter.

Pass the ResponseWriter into the HttpResponseMaker. The injection framework should then get the writer into the maker.

Use the writer to write the response to the outputstream.

Pass the output stream to the response builder

@archolewa I have trouble making ResponseWriter injectable.

The default ResponseWriter requires ResponseWriterSelector which requires all three pre-defined ResponseWriter to be instantiated (JSON, JsonApi and Csv) at binding time. But JsonResponseWriter and JsonApiResponseWriter needs two variables: responseData and paginationLinks that are not available at binding time. Is there a way to work around this.

tarrantzhang · 2017-08-02T18:22:55Z

Most line changes are due to refactoring large test cases and Response class, the number itself looks scary though.

archolewa · 2017-08-08T15:31:57Z

@kevinhinterlong I agree that having custom response types would be nice to have, but it's outside the scope of this PR. When someone wants custom response types, they can come in and make the refactors to the response type (i.e. hiding the enum behind an interface) that will be necessary.

kevinhinterlong · 2017-08-08T15:37:33Z

@archolewa I agree I think this PR is ready. I'll go ahead and make an issue for this

Rework on response serialization logic, and allow customizable response.

tarrantzhang · 2017-08-08T16:03:17Z

@archolewa @kevinhinterlong Rebased and squashed, ready for merge.

Rework on response serialization logic, and allow customizable response.

yahoo deleted a comment Jul 28, 2017

tarrantzhang force-pushed the refactor-httpresponsemaker-part2 branch from 34fd04f to 18f15ce Compare July 28, 2017 22:13

yahoo deleted a comment Jul 28, 2017

yahoo deleted a comment Jul 31, 2017

tarrantzhang commented Jul 31, 2017

View reviewed changes

archolewa requested changes Jul 31, 2017

View reviewed changes

yahoo deleted a comment Aug 2, 2017

yahoo deleted a comment Aug 7, 2017

yahoo deleted a comment Aug 8, 2017

address comments

f044062

Rework on response serialization logic, and allow customizable response.

tarrantzhang force-pushed the refactor-httpresponsemaker-part2 branch from 1f153cc to f044062 Compare August 8, 2017 15:51

yahoo deleted a comment Aug 8, 2017

archolewa merged commit fdadc11 into yahoo:master Aug 8, 2017

saiyan86 pushed a commit to saiyan86/fili that referenced this pull request Aug 28, 2017

Fili responses can now be customized (yahoo#455)

c07c344

Rework on response serialization logic, and allow customizable response.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Response creation part2 #455

Custom Response creation part2 #455

tarrantzhang commented Jul 28, 2017 •

edited

Loading

tarrantzhang Jul 31, 2017 •

edited

Loading

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa left a comment

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017 •

edited

Loading

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

archolewa Jul 31, 2017

tarrantzhang Aug 2, 2017

tarrantzhang commented Aug 2, 2017

archolewa commented Aug 8, 2017

kevinhinterlong commented Aug 8, 2017

tarrantzhang commented Aug 8, 2017 •

edited

Loading

Custom Response creation part2 #455

Custom Response creation part2 #455

Conversation

tarrantzhang commented Jul 28, 2017 • edited Loading

tarrantzhang Jul 31, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archolewa Jul 31, 2017 • edited Loading

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

tarrantzhang commented Aug 2, 2017

archolewa commented Aug 8, 2017

kevinhinterlong commented Aug 8, 2017

tarrantzhang commented Aug 8, 2017 • edited Loading

tarrantzhang commented Jul 28, 2017 •

edited

Loading

tarrantzhang Jul 31, 2017 •

edited

Loading

archolewa Jul 31, 2017 •

edited

Loading

tarrantzhang commented Aug 8, 2017 •

edited

Loading