StarpTech / apollo-datasource-http

Optimized JSON HTTP Data Source for Apollo Server
MIT License
73 stars 32 forks source link

Error upon receiving a compressed response. #38

Closed JustinTRoss closed 2 years ago

JustinTRoss commented 2 years ago

When you pass "Accept-Encoding" header to a supporting endpoint, and it responds with compressed data, it errors during json parse, here.

I'm having trouble finding a mechanism to decorate with this decompression. Any suggestion?

StarpTech commented 2 years ago

Hi @JustinTRoss, you should not pass this header but to support this use case we could check if the endpoint responds with a "Content-Encoding" header. If the response was compressed, we skip JSON parsing and pass the content as it is. Would you like to create a PR?

JustinTRoss commented 2 years ago

When you say, "you should not pass this header", do you mean "passing that header is not currently supported", or "passing that header is generally a bad practice" ? 😅

You suggestion could work. I figure we'd just switch on Content-Encoding and decode as appropriate, like so: https://github.com/nodejs/undici/discussions/1155#discussioncomment-2005131

StarpTech commented 2 years ago

I mean it is not supported. I'm not sure if it is a good idea to make this lib responsible for decompressing. It should be enough to get access to the raw body.

JustinTRoss commented 2 years ago

Got it. What gives you pause about doing so? Thinking through where we could handle compression, here are the thoughts that lead me to identify this as the place to handle it:

Something that may mitigate worry is to provide an escape hatch to communicate desire to prevent decompression, similar to what is used for fetch:

fetch(url, {
    compress: false,
    headers: { "accept-encoding": "gzip" },
});
StarpTech commented 2 years ago

I see your points and I agree with most of them but this library isn't a general-purpose HTTP client. It was developed for internal communication. Just for clarification. You request compression support right? If your API compresses content by default this is a violation of the protocol.

If you create a PR that adds compression support similar to

You suggestion could work. I figure we'd just switch on Content-Encoding and decode as appropriate, like so: https://github.com/nodejs/undici/discussions/1155#discussioncomment-2005131

I'd be happy to review it. Please have in mind that this library is optimized for performance. Requests that don't require decompression should not be impacted by any means.

JustinTRoss commented 2 years ago

Got it. Makes sense. Yes, compression was requested 😅 .

Definitely agreed regarding not impacting requests without a requirement for compression.

@jsudhakar-gdrx opened a PR that achieve this end. 🎉