search

adsabs / adsws

ADS web services
Other
2 stars 15 forks source link

Feature / Issue #89: Forward custom headers #90

Closed jonnybazookatone closed 8 years ago

jonnybazookatone commented 8 years ago

Added headers in the ProxyView response, such that custom headers returned by a micro-service are retained. This is useful in certain scenarios where we may want to return zip files, or other packets that are not necessarily JSON.

Temporarily fixed tests that were failing in test_discoverer. This was due to a dependency conflict. Further effort will be needed to figure out why this really caused there to be an issue, for now the version has been fixed.

jonnybazookatone commented 8 years ago

References #89.

jonnybazookatone commented 8 years ago

@romanchyla: notice any issues?

jonnybazookatone commented 8 years ago

Can probably include and close #88 here if you have a preference of how it should be.

jonnybazookatone commented 8 years ago

bump @romanchyla. Gonna push this to at least staging tomorrow, unless you have issues.

romanchyla commented 8 years ago

seems OK to me - there exists a potential for a microservice to return (overwrite?) headers that is should not return/overwrite. Consider having a whitelist (I don't feel strongly about it though)

the #88 will probably have to be addressed too - because if one wants to return an image/csv - the current code won't let that happen.

jonnybazookatone commented 8 years ago

Great, thanks for looking - I'll think about the white list a little bit.

Shouldn't #88 only affect outgoing messages via POST, DELETE, and PUT, and not affect the returning message from the microservice? I think it should be fine if you receive a content-type of image/csv from the microservice, you just couldn't send it anything but json.

jonnybazookatone commented 8 years ago

Oh, I guess my message was also a little confusing. I didn't mean this PR closes #88, just that when I posted it originally, I had the time to put in the changes to close #88 as well. So don't worry about discussing #88 here ;-).

romanchyla commented 8 years ago

I haven't realized that 3 out of 4 methods are using that call! But it is still relevant. It would be better if all of them behaved the same way. Some microservices actually allow POST requests (where you would normally use GET -- the endpoint doesn't change anything, but the input is too big for GET so we use POST). I imagine it would at least cause confusion, that headers get ignored sometimes.

jonnybazookatone commented 8 years ago

For now I'll not include a white-list. Given we have little restrictions on local module loading, any white listing would also have to propagate there.

If this is distasteful, we can remove it on another PR.