OpenTreeOfLife / feedback

No code -- just an issue tracker for general feedback (sent here via GitHub's issues API)
1 stars 0 forks source link

incorrect content type header #421

Closed fmichonneau closed 5 months ago

fmichonneau commented 6 years ago

At least from https://api.opentreeoflife.org/v3/tree_of_life/induced_subtree

the headers of the response contain "content-type: text/html" while it used to return "application/json".

mtholder commented 6 years ago

@bredelings I think fixing this just entails adding a renderer='json' to the routing in ws_wrapper, but I suspect that you'd know the optimal place to make that change. Can you take a look at that?

bredelings commented 6 years ago

Sure, taking a look...

bredelings commented 6 years ago

This should be fixed now.

After fixing the content-type in otcetera (which we need to do anyway), no further changes were required in ws_wrapper.  Whew.

BTW, I think the old server returned JSON also for non-200 responses: something like { "error" = "message" }

We're not doing this at the moment - we send back error messages as text, and then set the content-type to text/html;.  For manually-generated curl calls on the command line this type of response looks a bit less ugly.  However, can you let us know if this causes an error message for scripts?  I don't think it would be that hard to change.

-BenRI

On 09/17/2018 12:52 PM, Mark T. Holder wrote:

@bredelings https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_bredelings&d=DwMCaQ&c=imBPVzF25OnBgGmVOlcsiEgHoG1i6YHLR0Sj_gZ4adc&r=yDQgdhRIVlGWlo1dehDX4FDHdLhgFHI_XNBo6vRcIuw&m=Hwhdt7a69YXX3ijayj1BbtTOVh1U94iMGJNvMC11TOA&s=g1tV4HKAA-W1FFZTPkyU6dy39-D--TEVPjRYsv-Xe6Y&e= I think fixing this just entails adding a |renderer='json'| to the routing in ws_wrapper, but I suspect that you'd know the optimal place to make that change. Can you take a look at that?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_OpenTreeOfLife_feedback_issues_421-23issuecomment-2D422089929&d=DwMCaQ&c=imBPVzF25OnBgGmVOlcsiEgHoG1i6YHLR0Sj_gZ4adc&r=yDQgdhRIVlGWlo1dehDX4FDHdLhgFHI_XNBo6vRcIuw&m=Hwhdt7a69YXX3ijayj1BbtTOVh1U94iMGJNvMC11TOA&s=1hktc2uY9OXu_CHnIZ8CVPBXDsipoSprTYqxXi_UIdE&e=, or mute the thread https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AAIj7BHxUPGDhYeLz6fyX7Fkl7drr58Kks5ub9M4gaJpZM4WsSts&d=DwMCaQ&c=imBPVzF25OnBgGmVOlcsiEgHoG1i6YHLR0Sj_gZ4adc&r=yDQgdhRIVlGWlo1dehDX4FDHdLhgFHI_XNBo6vRcIuw&m=Hwhdt7a69YXX3ijayj1BbtTOVh1U94iMGJNvMC11TOA&s=5tVC3dVJGHNiW40gg7KN-vApe4KaOuTCS-kh_H1P7TY&e=.

mtholder commented 6 years ago

I do think it is probably best to return JSON either way. I suspect that some clients would prefer to just unpack the response body as JSON before checking for the status_code. I'm not sure if the API doc explicitly say that we should wrap the messages as JSON, but it seems to do whatever treemachine did

bredelings commented 6 years ago

Sure, yeah, I'll change this tomorrow to make it easier on clients.

bredelings commented 6 years ago

Hmm. So technically, I think a single string is now valid JSON. However, I implemented error-messages-as-JSON by returning an object instead:

{ "message": "[/v3/tree_of_life/node_info] Error: expecting a string argument called \"node_id\"\n" }

But we could just return a string.

jar398 commented 6 years ago

I think backward compatibility might be a consideration (phylotastic)

bredelings commented 6 years ago

Yeah. Old JSON parsers might not like a single string. Also it looks like tree machine uses the same format - an object with a "message" field.

I suppose we don't know if they ever parsed the error messages. But if you want to return a message to user, one might do so.