Apollo-server: apollo-datasource-rest parseBody better check json

[alexluong]

Hi. I noticed that apollo-datasource-rest parse my response's body as text. I checked with the source code, and this is how the library is currently checking:

  protected parseBody(response: Response): Promise<object | string> {
    const contentType = response.headers.get('Content-Type');
    if (
      contentType &&
      (contentType.startsWith('application/json') ||
        contentType.startsWith('application/hal+json'))
    ) {
      return response.json();
    } else {
      return response.text();
    }
  }

I was able to override that by changing the if condition with contentType.includes("json"). I wonder if we want to change the library itself, since there are APIs that respond with different content type header.

My use case is Contentful API, which return application/some-contentful-meta-data+json

[christianrondeau]

An even more classic case if the mime type text/json, which might not be "optimal" but is still valid!

I'm not sure about accepting "json" in generate, but at least not having that hardcoded and instead having it as a variable that can be overriden in the service would be nice.

What do you think?

[alexluong]

That would work. Maybe similar to baseURL. Or just document it a bit better I guess. I had to dig into the source code to figure out how to fix it (which is write an override parseBody function). Not the hardest thing in the world, but better documentation would help future devs running into problems like I did.

[christianrondeau]

Thanks @alexluong I did the same and it solved my problem, even though I don't like overriding their methods like this, since I'll lose any changes / future improvements to it.

I would suggest to at least move forward these options:

1. Add a property jsonContentTypes: string[] to RESTDataSource, to which we can add values to in the constructor like this: this.jsonContentTypes.push('text/json')
2. Make a method deserializeCustomContentType that can be overriden, and that returns ".text()" by default and nothing else. So we can deserialize anything that's not built-in easily.

Would one of these be accepted in a PR? Is there any preference?

[christianrondeau]

So a PR is ready to go: #2013 - however I did not create a deserializeCustomContentType since the vast majority of uses cases will not require doing anything else than JSON, and in these cases overriding parseBody isn't that bad. So it's very simple (and handles text/json out of the box)

[abernix]

This has been discussed recently on https://github.com/apollographql/apollo-server/pull/1645#issuecomment-439370832.

As parseBody is a protected method (that is to say, it can be implemented by any class which extends RESTDataSource), developers are free to implement override its default behavior in any way they'd like — including and up to allowing any Content-Type which contains json as necessitated above:

class DogAPI extends RESTDataSource {
  constructor() {
    super();
    this.baseURL = "https://dog.ceo/api/";
  }

  parseBody(response) {
    if (response.headers.get('Content-Type').includes('json')) {
      return response.json();
    } else {
      return response.text();
    }
  }

  /* ... custom RESTDataSource methods ... */
}

Also, if we were to go the route of jsonContentTypes, we'd quickly end up with a long laundry list of other content-types (e.g. xmlContentTypes, protobufContentTypes, etc.).

I believe this provides the best abstraction without overlooking specific implementation details (and often time, meaningful differences) between various content types which might appear similar on the surface.

[christianrondeau]

Works with me, thanks for looking into this! (text/jon is a "legacy" mime type for JSON, so it's fine if we have to work around it)

[vincent-vade]

This has been discussed recently on #1645 (comment).

As parseBody is a protected method (that is to say, it can be implemented by any class which extends RESTDataSource), developers are free to implement override its default behavior in any way they'd like — including and up to allowing any Content-Type which contains json as necessitated above:

class DogAPI extends RESTDataSource {
  constructor() {
    super();
    this.baseURL = "https://dog.ceo/api/";
  }

  parseBody(response) {
    if (response.headers.get('Content-Type').includes('json')) {
      return response.json();
    } else {
      return response.text();
    }
  }

  /* ... custom RESTDataSource methods ... */
}

Also, if we were to go the route of jsonContentTypes, we'd quickly end up with a long laundry list of other content-types (e.g. xmlContentTypes, protobufContentTypes, etc.).

I believe this provides the best abstraction without overlooking specific implementation details (and often time, meaningful differences) between various content types which might appear similar on the surface.

Thank's