Feat: Add custom body serializer support #103

paulpdaniels · 2018-04-26T03:37:39Z

Adds support for custom body serializers which can be specified in a similar manner to bodyBuilder. Defaults to JSON if none is specified in order to preserve default behaviour.

cc/ @fbartho

fixes #96

apollo-cla · 2018-04-26T03:37:40Z

@paulpdaniels: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

paulpdaniels · 2018-04-30T02:48:40Z

CI seems to be reporting an issue when packaging the build, due to something in graphql if I am not mistaken? Perhaps the package is getting pulled in twice?

> apollo-link-rest@0.2.4 build /home/travis/build/apollographql/apollo-link-rest
> tsc -p .
src/restLink.ts(859,49): error TS2345: Argument of type 'DocumentNode' is not assignable to parameter of type 'DocumentNode'.
  Types of property 'loc' are incompatible.
    Type 'Location' is not assignable to type 'Location'. Two different types with this name exist, but they are unrelated.
      Types of property 'startToken' are incompatible.
        Type 'Token' is not assignable to type 'Token'. Two different types with this name exist, but they are unrelated.
          Types of property 'kind' are incompatible.
            Type 'TokenKind' is not assignable to type '"<SOF>" | "<EOF>" | "!" | "$" | "(" | ")" | "..." | ":" | "=" | "@" | "[" | "]" | "{" | "|" | "}"...'.
              Type '"BlockString"' is not assignable to type '"<SOF>" | "<EOF>" | "!" | "$" | "(" | ")" | "..." | ":" | "=" | "@" | "[" | "]" | "{" | "|" | "}"...'.
src/restLink.ts(885,53): error TS2345: Argument of type 'DocumentNode' is not assignable to parameter of type 'DocumentNode'.
src/restLink.ts(887,46): error TS2345: Argument of type 'DocumentNode' is not assignable to parameter of type 'DocumentNode'.
src/restLink.ts(888,56): error TS2345: Argument of type 'DocumentNode' is not assignable to parameter of type 'DocumentNode'.

damusnet · 2018-05-07T00:05:04Z

Same issue as apollographql/apollo-link#601 (comment) ; check a2514eb for solve. CI would pass once #105 is merged, and this MR rebased.

fbartho

This looks great, however, can you please add documentation to docs/rest.md ?

Thanks @paulpdaniels!

codecov-io · 2018-05-09T04:23:13Z

Codecov Report

Merging #103 into master will increase coverage by 0.39%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   90.64%   91.03%   +0.39%     
==========================================
  Files           2        2              
  Lines         278      279       +1     
  Branches       89       87       -2     
==========================================
+ Hits          252      254       +2     
+ Misses         23       22       -1     
  Partials        3        3

Impacted Files	Coverage Δ
src/restLink.ts	`91% <95.65%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a74033e...015d824. Read the comment docs.

paulpdaniels · 2018-05-09T05:10:59Z

@fbartho docs added.

paulpdaniels · 2018-05-11T09:28:53Z

Actually don't merge (quite) yet, I'm fairly certain I put the docs in the wrong place.

marnusw · 2018-05-11T09:42:19Z

I noticed that the Content-Type header isn't set correctly when using the default bodyBuilder/JSON serializer. I am wondering if bodySerializer shouldn't be given access to the headers to allow for this? I know headers can be configured in a variety of ways already, but I think if any function changes the body encoding it should also change the header.

How about changing the signature to bodySerializer: (body: any, headers: Headers) => bodyToSend. That way the Content-Type header could be updated?

This would mean extending the default body serialization to set the header to application/json as well.

(As a side note, the bodyBuilder docs on the Apollo website should probably be fixed when this option is added there, as it shouldn't be encoding the body that is returned.)

fbartho · 2018-05-14T06:26:13Z

@marnusw I concur with your recommendations. -- Is there a better signature we can provide than that one though?. This feels a bit brittle, what if we want more parameters passed in down the line? -- Maybe we should make it accept options, and return { body, headers, … } ? -- If we're breaking the signature in this, we should definitely make it worth it.

fbartho · 2018-05-14T06:28:42Z

@marnusw re: your comment about the docs: -- we have the docs mirrored in this repo, so any changes we make to release the next release of apollo-link-rest, we can mirror back into the official repo.

fbartho · 2018-05-14T07:09:24Z

@paulpdaniels please flag when you're ready for us to review things again after your comment that you weren't quite ready!

marnusw · 2018-05-14T20:13:15Z

src/restLink.ts

+
+    /**
+     * Optional field that prepares the request body for transport.
+     * @default function that serializes from JSON.


Typo: Should be "serializes to JSON"

marnusw · 2018-05-14T20:41:01Z

Maybe we should make it accept options, and return { body, headers, … } ? -- If we're breaking the signature in this, we should definitely make it worth it.

That makes sense. I looked at it again, and right now I can't see any additional options that would be worth passing in, but changing the signature in this way will make that possible down the line:

So for now I would recommend bodySerializer: ({ body: any, headers: Headers }) => object | string.

If I may, pending consensus on the signature above, these are the summarized recommendations we've discussed:

The bodySerializer should also receive the request headers and set the Content-Type header.
The default bodySerializer should convert to JSON and set Content-Type: application/json.
Correct the bodyBuilder docs which states it can be used for serialization on line 265 of docs/rest.md.

paulpdaniels · 2018-05-16T02:14:32Z

@marnusw I like your idea, wondering if we could tweak it though to keep bodySerializer referentially transparent, by simply having the method return headers. Then we could (possibly?) reuse the headersMergePolicy and merge the new headers with the existing ones.

const jsonSerializer = (body: args) => ({data: JSON.stringify(body), headers: {'ContentType': 'application/json'}})


// In restLink

const {data, headers: additionalHeaders} = body ? (bodySerializer || jsonSerializer).call(null, body) : {};

const mergedHeaders = headerMergePolicy(headers, additionalHeaders)

    return await (customFetch || fetch)(`${uri}${pathWithParams}`, {
      credentials,
      method,
      headers: mergedHeaders,
      body: data
    })

Would that work @fbartho ?

marnusw · 2018-05-16T08:00:00Z

@paulpdaniels That sounds like a pretty elegant solution. I didn't actually check now, but I'm guessing there would be no reason why the body serializer couldn't be called before the initial header merge, and then there would potentially be just one header merge step from the three different sources.

I have to add, though, that I intended to come back and post saying it's interesting how your ideas mellow over a few days: It occurred to me that as far as actual usage of these features go the following makes pretty good sense:

new ApolloLinkRest({
  headers: {'Content-Type': 'multipart/form-data'},
  bodySerializer = (body: args) => /* Make FormData */)
})

It's concise and does exactly what is needed and expected. The only other thing that would be required then is that the lib sets the Content-Type: application/json header internally if it has not been overridden by other header sources. My main reason for entering this discussion was simply that the lib currently sends a JSON body with the wrong header value, the rest stemmed from trying to make that work well along with this new feature.

paulpdaniels · 2018-05-16T14:06:39Z

@marnusw Do you mean for setting the default serializer and headers? Because I think we need to set the serializer per @rest call since each one could have a different serializer. Currently the initial merge is done in the request method while the serialization is done via during the resolver step. i.e.

Something like:

mutation Signup(
  $formData: FormDataInput, 
  $loginData: LoginDataInput,
  $formSerializer: any,
  $jsonSerializer: any
) {
  signup(input: $formData) @rest(type: "Signup", path: "/v1/signup", bodySerializer: $formSerializer) { ...SignedUpFragment }
  login(input: $loginData) @rest(type: "User", path: "/v1/login", bodySerializer: $jsonSerializer) { ...UserFragment }
}

We need to pass a serializer for each.

You did get me thinking though that maybe we could support something like how the endpoints field works, we could pass in an object bag with the available serializers (since there are unlikely to be more than a couple). And then the @rest directive can select one by key.

new RestLink({
  ...standardOptions,
  additionalSerializers: {
    // Don't need to include JSON unless we are overriding it, it will also be our default
    form: (body: any) => ({data: buildForm(body), headers: {'Content-Type': 'multipart/form'}})
  }
})

marnusw · 2018-05-16T14:31:17Z

@paulpdaniels Got it, that makes perfect sense. I hadn't looked at the implementation in sufficient detail to notice you were doing it per @rest call.

I think defining the available serializers once in the initial config as you propose is a good idea since it could become repetitive otherwise. In this case each function setting its own headers is definitely a requirement as well, with either one of the two ways discussed. Returning the headers does keep the function pure which I like.

I just noticed as far as naming things go we should probably pass in data and return body and headers in line with the fetch naming convention:

  form: (data: any) => ({body: buildForm(data), headers: {'Content-Type': 'multipart/form'}})

Let's see what @fbartho thinks.

fbartho · 2018-05-16T19:21:20Z

Hey guys, I love this thread. Here are my first thoughts on the discussion:

yay!
I love that the builder receives the data, but it doesn't receive the "current" header stack -- is that intentional? or should the serializer also receive the header stack as input? -- Headers can be tweaked by apollo-link-context etc, so maybe that's a desirable input.
I tried to suggest referential transparency in my comment here: Feat: Add custom body serializer support #103 (comment) haha, but I guess I should have used that terminology to make it clear @paulpdaniels
If we do return the headers, we need to make sure that the headersMergePolicy interacts correctly across the levels: Link, Context, bodyBuilder, bodySerializer, Fetch + How Request-options merges in there, plus how @rest(…) directives can fit in.
I'm happy providing a hash of individual extra serializers at the RestLink level -- but then is the interface to pass a bodySerializerKey to the @rest() annotation to select it? -- Or if the bodySerializer is a string, then we know we need to look it up? -- I didn't fully follow the recommendation.
For base architectural style, we try to let people reconfigure @rest(…) calls in the directive at query time as much as possible. I don't want to close the door on that. -- To put it another way, I like @paulpdaniels's first example: Feat: Add custom body serializer support #103 (comment)

Did I forget to address anything? haha didn't mean to throw a wall of text at you. /cc @marnusw

paulpdaniels · 2018-05-17T02:44:31Z

Ok @fbartho @marnusw I tried to boil down what I think we are all saying. I refactored so that there are now a couple options for handling custom body serializers (I followed the precedent with bodyKey and bodyBuilder as well as uri and endpoints). For one off custom bodies, lets say you are interacting with a 3rd party form API, there is the bodySerializer which is exactly what was specified above. If people find that too verbose or plan to be switching a lot there is also the bodySerializerKey now which will be a string-based selector that will select a serializer from an object config passed in during construction of the RestLink. Finally, for those who have to deal with a legacy code base or want a different default, you can now also specify the defaultSerializer when constructing the RestLink.

All together it would look something like this (apologizes for the contrived example):

const link = new RestLink({
  // Pre-specify some additional serializers
  additionalSerializers: {
    json: data => ({body: JSON.stringify(data), headers: {'Content-Type': 'application/json'}})
  },
  // If not specified then always use xml
  defaultSerializer: xmlSerializer
})

const mutation = gql`
  mutation OneForAll($input: FormInput!, $customSerializer: any) {
    # Use a custom one off serializer
    postForm(input: $input) @rest(
      path: "/v1/form", 
      type: "Response", 
      method: "POST", 
      bodySerializer: $customSerializer
    )
    {  
      id, timestamp
    }

    # Uses the default serializer 
    postDefault(input: $input) @rest(
      path: "/v1/xml", 
      type: "Response", 
      method: "POST"
    ) 
    { ...DefaultFragment }

    # Use the serializer based on the passed in key    
    postJSON(input: $input) @rest(
      path: "/v1/json", 
      type: "Response", 
      method: "POST",
      bodySerializerKey: 'json'
    ) { id }
  }
`;


client.mutate({
  query: mutation,
  variables: {
    input: {hello: "world"},
    // Arbitrary constant serializer
    customSerializer: () => ({body: 42, headers: {'Content-Type': 'text/plain'}})
  }
})

If we agree on these features, I'll fix the test and update the docs based on these options.

paulpdaniels · 2018-05-17T02:51:41Z

src/restLink.ts

-      headers,
-      body: body && JSON.stringify(body),
+      headers: additionalHeaders
+        ? headersMergePolicy(additionalHeaders, headers)


This I'm actually not to sure about @fbartho, if I switch the order here, which I initially tried to do, it broke some other non-related tests. Doing this keeps all tests passing, but I guess we should decide what the expected behavior of this should be? I assume the serializer should in general get the final say here, but I think this might be doing the opposite no?

paulpdaniels · 2018-05-17T02:58:46Z

@fbartho per your point 2, I guess it would depend on how we handle header merging. I'm not certain how often a serializer would need the current header stack to conditionally determine what to do. On its surface it seems like it would be helpful to expose that to serializer, but can't think of a use case to actually justify it.

marnusw · 2018-05-18T13:08:05Z

@paulpdaniels, I think this is a good proposal. It's grown somewhat in complexity, but it does provide a lot of flexibility.

I just have some opinions about how things are named:

1 - In the RestLink options I propose changing additionalSerializers -> bodySerializers. The docs could indicate that these are in addition to json, which should be clear enough, and then the option name will describe the actual intent.

2 - I think the serializer key format will be more common than a local custom serializer, so how about using the shorter label bodySerializer for providing a key into the bodySerializers bag. And when a local function is provided that could be on the customSerialzer option. So:

bodySerializerKey -> bodySerializer
bodySerializer -> customSerialzer  // Although this does sound slightly ambiguous

I don't feel strongly about point 2 though, just a thought.

In terms of passing the headers into the serializer function: I also can't think of a use case for that right now. It think the function will/should mainly be used to set the content-type, and maybe an authorization header for a particular API. Even the last seems like it's not quite the place for it, but at the moment modifying headers per @rest call isn't possible, and this will open up that possibility which some may find useful. But back to the point, I don't think any actions in the serializer will typically depend on the current headers.

paulpdaniels · 2018-05-19T10:35:44Z

@marnusw 👍 on the naming conventions, I'll update those. I'm going to leave the serializer signature as is for now, i.e. not passing in the headers to it, just merging the resulting headers, if only because neither of us have a clear reason why it'd be necessary to include that ability. I think it may be better to just defer that decision (since it would be backward compatible), to when someone comes forward with an articulated use case for this. Then we can have PR that specifically addresses that issue.

fbartho · 2018-05-19T22:13:53Z

How do we feel about bodySerializer being either a function or a string key? That prevents us from adding yet another potential parameter, while just making the link code a little harder to write, (but TypeScript covers that for us).

The reason why we need to pass the headers in, is that headers are by REST spec allowed to have duplicates. But if there is already a content-type header, then we need to replace it with the new content-type coming out of the bodySerializer. Aka, the bodyserializer needs to be able to replace the headers wholesale, not just have its headers merged in.

fbartho · 2018-05-19T22:15:03Z

I’m ready to come to a final version and merge this today or tomorrow if you @paulpdaniels or @marnusw think we can come to a happy solution that is fully implemented by tomorrow?

paulpdaniels · 2018-05-21T02:34:02Z

Ok @fbartho incorporated your feedback as well.

fbartho · 2018-05-21T02:59:52Z

A quick scan of the latest changes look like exactly what I had been thinking of @paulpdaniels, thanks! -- I'll try to carve out some time to test it tomorrow!

marnusw

Thanks for finishing the implementation @paulpdaniels! I haven't looked at the code yet, but I have a few comments on the docs.

marnusw · 2018-05-22T07:03:05Z

docs/rest.md

@@ -90,6 +90,9 @@ Construction of `RestLink` takes an options object to customize the behavior of
 * `fieldNameNormalizer?: /function/`: _optional_ function that takes the response field name and converts it into a GraphQL compliant name. -- This is useful if your `REST` API returns fields that aren't representable as GraphQL, or if you want to convert between `snake_case` field names in JSON to `camelCase` keyed fields.
 * `fieldNameDenormalizer?: /function/`: _optional_ function that takes a GraphQL-compliant field name and converts it back into an endpoint-specific name.
 * `typePatcher: /map-of-functions/`: _optional_ Structure to allow you to specify the `__typename` when you have nested objects in your REST response!
+* `bodySerializers: /map-of-functions/`: _optional_ Structure to allow the definition of alternative serializers, which can then be specified by their key.
+* `defaultSerializer /function/`: _optional_ function that will be used by the `RestLink` as the default serializer when no `bodySerializer` is defined for a `@rest` call. Default is JSON.


If I may nitpick, could defaultSerializer and bodySerializers be swapped around here? The way I read the narrative it would be easier to understand the intent with defaultSerializer first and then discussing the even more advanced configuration.

marnusw · 2018-05-22T07:04:14Z

docs/rest.md

@@ -192,6 +195,13 @@ Here is one way you might customize `RestLink`:
        bodySnippet...
      }
    },
+    bodySerializer: (body: any) => {


I think this option should be defaultSerializer now. Should it also be receiving the headers as a second parameter and returning {body, headers}?

Yep you are right!

marnusw · 2018-05-22T07:07:59Z

docs/rest.md

@@ -263,6 +273,7 @@ An `@rest(…)` directive takes two required and several optional arguments:
 * _optional_ `endpoint?: string` key to use when looking up the endpoint in the (optional) `endpoints` table if provided to RestLink at creation time.
 * _optional_ `bodyKey?: string = "input"`: This is the name of the `variable` to use when looking to build a REST request-body for a `PUT` or `POST` request. It defaults to `input` if not supplied.
 * _optional_ `bodyBuilder?: /function/`: If provided, this is the name a `function` that you provided to `variables`, that is called when a request-body needs to be built. This lets you combine arguments or encode the body in some format other than JSON.
+* _optional_ `bodySerializer?: /function/`: function that takes the body of the request directly before it is passed to the fetch call. Defaults to `JSON.stringify`.


This can be a function or a string now, for indexing into bodySerializers. Perhaps:

optional bodySerializer?: /string | function/: string key to look up a function in bodySerializers or a custom serialization function for the body/headers of this request before it is passed to the fetch call. Defaults to JSON.stringify and setting Content-Type: application/json.

marnusw · 2018-05-22T07:49:07Z

docs/rest.md

+```graphql
+mutation encryptedForm(
+  $input: PublishablePostInput!,
+  formSerializer: any


This is just a question for my own sake: Why is there no $ in front of this variable? Does that tell GraphQL not to serialize it (since it's a function)? Would it still be passed in on the variables same as the input value?

I'm also curious! :)

Ah lol, that would be a typo.

O, right... I just thought it mightn't be because it was like that twice. Copy & paste I guess. 😄

fbartho · 2018-05-23T08:46:24Z

Just one more issue to solve including this PR (#102)

paulpdaniels · 2018-05-23T09:42:08Z

@fbartho @marnusw comments addressed, though it appears I rebroke the build by rebasing...

fbartho

I don't think the build problems are your fault. I think they're something wrong between apollo-utilities and the graphql dependencies.

I'm going to merge this and try to fix those separately.

ghost added feature Feature: new addition or enhancement to existing solutions labels Apr 26, 2018

fbartho requested changes May 7, 2018

View reviewed changes

fbartho added this to the v0.3.0 milestone May 7, 2018

paulpdaniels force-pushed the support-customer-serializers branch from f428e93 to bf3f426 Compare May 9, 2018 04:23

This was referenced May 9, 2018

Move dependencies to peerDependencies #105

Merged

Body is always JSON encoded, even with bodyBuilder set #108

Closed

marnusw reviewed May 14, 2018

View reviewed changes

paulpdaniels force-pushed the support-customer-serializers branch from bf3f426 to 62f34fc Compare May 15, 2018 13:01

paulpdaniels commented May 17, 2018

View reviewed changes

marnusw reviewed May 22, 2018

View reviewed changes

paulpdaniels added 2 commits May 23, 2018 17:24

Feat: Add custom body serializer support

767f99d

chore: add bodySerializer docs

22cdde7

paulpdaniels force-pushed the support-customer-serializers branch from a74a84f to 015d824 Compare May 23, 2018 09:24

paulpdaniels added 3 commits May 23, 2018 17:25

Feat: Support bodySerializerKey and bodySerializer

4b959f8

Refactor: Merge into bodySerializer

e078621

Chore: Fix bodySerializer docs

015d824

fbartho self-requested a review May 25, 2018 21:11

fbartho approved these changes May 25, 2018

View reviewed changes

fbartho merged commit 015d824 into apollographql:master May 25, 2018

Feat: Add custom body serializer support #103

Feat: Add custom body serializer support #103

Conversation

paulpdaniels commented Apr 26, 2018 • edited Loading

apollo-cla commented Apr 26, 2018

paulpdaniels commented Apr 30, 2018

damusnet commented May 7, 2018

fbartho left a comment

Choose a reason for hiding this comment

codecov-io commented May 9, 2018 • edited Loading

Codecov Report

paulpdaniels commented May 9, 2018

paulpdaniels commented May 11, 2018

marnusw commented May 11, 2018 • edited Loading

fbartho commented May 14, 2018

fbartho commented May 14, 2018

fbartho commented May 14, 2018

Choose a reason for hiding this comment

marnusw commented May 14, 2018

paulpdaniels commented May 16, 2018

marnusw commented May 16, 2018

paulpdaniels commented May 16, 2018 • edited Loading

marnusw commented May 16, 2018

fbartho commented May 16, 2018

paulpdaniels commented May 17, 2018

Choose a reason for hiding this comment

paulpdaniels commented May 17, 2018

marnusw commented May 18, 2018

paulpdaniels commented May 19, 2018

fbartho commented May 19, 2018

fbartho commented May 19, 2018

paulpdaniels commented May 21, 2018

fbartho commented May 21, 2018

marnusw 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

marnusw May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marnusw May 23, 2018 • edited Loading

Choose a reason for hiding this comment

fbartho commented May 23, 2018

paulpdaniels commented May 23, 2018

fbartho left a comment

Choose a reason for hiding this comment

paulpdaniels commented Apr 26, 2018 •

edited

Loading

codecov-io commented May 9, 2018 •

edited

Loading

marnusw commented May 11, 2018 •

edited

Loading

paulpdaniels commented May 16, 2018 •

edited

Loading

marnusw May 22, 2018 •

edited

Loading

marnusw May 23, 2018 •

edited

Loading