Option to specify responseTransformer per @rest directive

[jtomaszewski]

We just started using apollo-link-rest for our app where we have multiple APIs with multiple endpoints with multiple queries and mutations.

Problem is, the APIs we fetch aren't of a perfect REST format, and sometimes it would be nice to transform the fetch response only of the given query/mutation.

For example:

• I'd like to transform response HTTP 200 that returns { error: true } and make it into something that returns 422 (so that even though it is successful in HTTP, in graphql it will be treated as an error)
• I'd like to transform response HTTP 400 that returns { errorCode: '_program_not_found' } into something that returns 200 and has different JSON (so that even though it is unsucessfull it http, in graphql it will be treated as a successful response)

Right now the only way to do it AFAIK is to replace the customFetch or add responseTransformer, but this can be done only once for the whole apollo-link-rest. But because we have many different endpoints, that responseTransformer would quickly end up bloated and unmaintainable if we'd keep there logic for transforming all the queries/endpoints (they differ a lot from each other).

So maybe we could pass a responseTransformer to the @rest directive, just like we currently do i.e. with pathBuilder or bodyBuilder ?

WDYT? I think I could even do a PR for that whole thing ;)

[jtomaszewski]

Also, it would be nice if responseTransformer wouldn't just transform the response body, but actually the whole response object.

Then, it would be possible to change the status code for example. (treat 404 as 200 and vice-versa)

[fbartho]

I would support a proposal to improve the API for responseTransformer to receive the whole response, but my challenge to you, is: Can you think of a way to do it in a backwards compatible way?

Regarding your idea/request to provide yet another optional parameter for the @rest( directive, I would ask you to please take a look at my comment here: #214 (comment)

Additionally, the example code in that comment would be similar to what you would need to do to make your own customFetch or custom responseTransformer to solve your problem too.

What do you think @jtomaszewski?

[jack-sf]

I would support a proposal to improve the API for responseTransformer to receive the whole response, but my challenge to you, is: Can you think of a way to do it in a backwards compatible way?

Hm. That's tricky, true.

Option A - add a new param and slowly deprecate old one

We could add a new parameter, like fullResponseTransformer. Keep the responseTransformer in the code, but hide it TS typedefs (or add a flag to it like @deprecated or @hidden or something) and add deprecation console.log message [that would be fired at most once per RestLink lifetime) if anybody is still using responseTransformer.

And then some time later in a next major version, drop the support for old responseTransformer.

And then again some time later, rename fullResponseTransformer to responseTransformer. (With keeping backwards compability for fullResponseTransformer for another major version).

Option B - add a boolean param

We could add a new boolean param to RestLink constructor like new RestLink({ useFullResponseTransformer: true }) that would switch the behaviour of responseTransformer param.

This param could have false as the default value, and then we could switch it to true in some next major version. (Or set it to true from the beginning, but let other users to switch it to false until they convert to the new full response transformer way).

WDYT?

There's another question what to do with this code https://github.com/apollographql/apollo-link-rest/blob/master/src/restLink.ts#L1035-L1068 . Should the full response transformer replace it (or happen before/after it) ?

Another option would be to keep both params, like have a separate responseTransformer (new one) and responseBodyTransformer (old one).

[jack-sf]

Regarding your comment #214 (comment) . Unfortunately I haven't thought of controlling the response body by the request url in customFetch method. So, for now we've done it by basing on the response body only. The code below is what we've accomplished for now. But I think it won't scale well and will be hard for us to maintain over time, as we plan to have a couple of different endpoints (with tens of queries and mutations) , where each single query/mutation might need a separate response transformer.

That's how it looks currently for us:

/**
 * Is the given error response a response that we should treat as successful one?
 * (See `customFetch` code comments for more details why would we do it.)
 */
function isSafeError(body: any): boolean {
  if (!body || body.type !== 'ERROR') {
    return false;
  }

  /**
   * "No segment found for the program"
   * Required for `useFindProgramSegmentId`
   * ( LINK_TO_API_DOCS )
   */
  if (body.code === '_no_segment_id_found') {
    return true;
  }

  return false;
}

const customFetch: typeof fetch = (url: RequestInfo, options?: RequestInit) => {
  return new Promise<Response>((resolve, reject) => {
    fetch(url, options).then(async response => {
      /**
       * XXAPI sometimes returns a meaningful error data,
       * together with 400 status,
       * when for us it's actually a response that we shoud treat as successful one, that is:
       * - `error` should be empty,
       * - `networkStatus` should mark it as done,
       * - response body should be cached by Apollo.
       *
       * Example:
       * In `useFindProgramSegmentId`,
       * request returns HTTP 400 with `_no_segment_id_found` error code,
       * when we would prefer it to return HTTP 200 with segmentId being an empty value.
       *
       * Because there's no option at the moment to define a responseTransformer
       * per given query ( https://github.com/apollographql/apollo-link-rest/issues/215 ),
       * let's define it here, and transform such responses into successful responses,
       * whenever they include one of these error codes,
       * and then put that error in the response under the `error` property.
       *
       * Also, because we still want to treat responses with other errors
       * (i.e. `_system_error`) as unsuccessful,
       * we'll do that whole operation only for the error codes defined in `SAFE_ERROR_CODES`.
       */
      if (response.status === 400) {
        let body: any;
        try {
          body = await response.clone().json();
        } catch (error) {
          // do nothing
        }
        if (isSafeError(body)) {
          const newResponse = response.clone();
          Object.defineProperty(newResponse, 'ok', {
            get: () => true,
          });

          resolve(newResponse);
          return;
        }
      }

      resolve(response);
    }, reject);
  });
};


/**
 * This is the continuation of the job from our `customFetch`.
 *
 * If the response is a `{ type: 'ERROR', ... }` object,
 * let's put it under the `error` property,
 * so it can be accessed like `response.data.error`
 * (`{ error: { type: 'ERROR', ... } }`)
 *
 * This makes it "easy" to retrieve the error code by Apollo by specifying the `error` property
 * in the GQL, so it can retrieve both successful and "error-but-successful" responses.
 */
const responseTransformer = async (
  response: Response | {} | null,
  _type: string,
) => {
  let body: any;

  if (response && (response as Response).json) {
    body = await (response as Response).json();
  }

  if (isSafeError(body)) {
    return {
      error: body,
    };
  }

  return body;
};

// and then, when creating RestLink:
    const restLink = new RestLink({
      endpoints: { ... },
      customFetch,
      responseTransformer,
    });

And then configuring the individual query for which the response is transformed in a way that it is successful and contains the error data (even though request returned 422):

export const FIND_PROGRAM_SEGMENT_ID_QUERY = gql`
  query RoomBookingAPI__FindProgramSegmentId($programId: String!) {
    programSegmentId(programId: $programId)
      @rest(
        type: "ProgramSegmentId"
        endpoint: "room-booking"
        method: "GET"
        path: "/v1/offers/room/segment/{args.programId}"
      ) {
      segmentId
      error
    }
  }
`;

interface FindProgramSegmentIdData {
  programSegmentId:
    | {
        segmentId: string;
        error: null;
      }
    | {
        segmentId: null;
        error: {
          type: 'ERROR';
          code: '_no_segment_id_found';
          msg: string;
        };
      };
}

/**
 * Returns program's segmentId (or `undefined`, if program doesn't have a segment).
 */
export const useFindProgramSegmentId = (
  options: QueryHookOptions<{
    programId: string;
  }>,
) => {
  return useQuery<FindProgramSegmentIdData>(
    FIND_PROGRAM_SEGMENT_ID_QUERY,
    options,
  );
}

As you can see, that's quite a bit of code to do a simple response transformer for one given endpoint. But the biggest pain in here is that the whole logic of transforming the response is controlled in our isSafeError, responseTransformer, customFetch functions, which are used by all the requests, thus quickly over time they will be bloated with a lot of if else statements.

That code could be a bit simplified if we went with your suggestion from #214 (comment) , and we could just serve a different customFetch method per each request url.

But then we would still need to maintain that registry of URLs (somebody would have to create the registry, and then if each request that needs a transformer would be created in another file, the registry file would need to require all those transformers and their urls and use them to create one big customFetch used by the RestLink).

So maybe the other question that we should focus first is: what's wrong with @rest having those couple optional parameters (like responseTransformer) that can be replaced whenever somebody wants to?

[jack-sf]

OK I think steps to do it could be as simple as following:

1. Keep the current behaviour of responseTransformer but hide it from TS typings and add responseBodyTransformer that would do the same thing and would be the preferred way if you want just to transform the response body.

2. Add fullResponseTransformer that can be specified per link and per endpoint:

interface FullResponseTransformerOptions {
  type?: string; 
  responseBodyTransformer: ResponseBodyTransformer, 
  buildServerSideError: (result: any, message: string) => RestLink.ServerError;
};

type FullResponseTransformer = (response: Response, options: FullResponseTransformerOptions) => object;

This method either returns a json or throws an error that is built using the options.buildServerSideError.

Then if you want to transform the response fully, you can easily replace fullResponseTransformer and reuse responseBodyTransformer and buildServerSideError if you want.

In https://github.com/apollographql/apollo-link-rest/blob/master/src/restLink.ts , we could replace

  let result;
  if (response.ok) {
    if (
      response.status === 204 ||
      response.headers.get('Content-Length') === '0'
    ) {
      // HTTP-204 means "no-content", similarly Content-Length implies the same
      // This commonly occurs when you POST/PUT to the server, and it acknowledges
      // success, but doesn't return your Resource.
      result = {};
    } else {
      result = response;
    }
  } else if (response.status === 404) {
    // In a GraphQL context a missing resource should be indicated by
    // a null value rather than throwing a network error
    result = null;
  } else {
    // Default error handling:
    // Throw a JSError, that will be available under the
    // "Network error" category in apollo-link-error
    let parsed: any;
    // responses need to be cloned as they can only be read once
    try {
      parsed = await response.clone().json();
    } catch (error) {
      // its not json
      parsed = await response.clone().text();
    }
    rethrowServerSideError(
      response,
      parsed,
      `Response not successful: Received status code ${response.status}`,
    );
  }

  const transformer = endpointOption.responseTransformer || responseTransformer;
  if (transformer) {
    // A responseTransformer might call something else than json() on the response.
    try {
      result = await transformer(result, type);
    } catch (err) {
      console.warn('An error occurred in a responseTransformer:');
      throw err;
    }
  } else if (result && result.json) {
    result = await result.json();
  }

with

function defaultFullBodyTransformer(response: Response, options: FullResponseTransformerOptions) {
  let result;
  if (response.ok) {
    if (
      response.status === 204 ||
      response.headers.get('Content-Length') === '0'
    ) {
      // HTTP-204 means "no-content", similarly Content-Length implies the same
      // This commonly occurs when you POST/PUT to the server, and it acknowledges
      // success, but doesn't return your Resource.
      result = {};
    } else {
      result = response;
    }
  } else if (response.status === 404) {
    // In a GraphQL context a missing resource should be indicated by
    // a null value rather than throwing a network error
    result = null;
  } else {
    // Default error handling:
    // Throw a JSError, that will be available under the
    // "Network error" category in apollo-link-error
    let parsed: any;
    // responses need to be cloned as they can only be read once
    try {
      parsed = await response.clone().json();
    } catch (error) {
      // its not json
      parsed = await response.clone().text();
    }
    throw options.buildServerSideError(
      response,
      parsed,
      `Response not successful: Received status code ${response.status}`,
    );
  }

  const bodyTransformer = options.responseBodyTransformer;
  if (bodyTransformer) {
    try {
      result = await bodyTransformer(result, options.type);
    } catch (err) {
      console.warn('An error occurred in a responseTransformer:');
      throw err;
    }
  } else if (result && result.json) {
    result = await result.json();
  }

  return result;
}

let result = endpointOption.fullResponseTransformer || fullResponseTransformer || defaultFullBodyTransformer;

@fbartho WDYT?

[nishanBende]

@jack-sf It'd be nice to have different response transformer per @rest, I had a requirement to integrate it with an existing project and the structure of the API responses differs from what Apollo expects. Arrays can be nested with a key and sometimes objects too.
I decided to give it a try this weekend but got in this issue. @fbartho I also saw your comment and its perfect to do so with customFetch but I strongly feel the response transformer should be per request.