openfisca / openfisca-web-api

[DEPRECATED] Web API for OpenFisca
https://www.openfisca.fr/
GNU Affero General Public License v3.0
13 stars 11 forks source link

Refactor middlewares #70

Closed cbenz closed 7 years ago

cbenz commented 7 years ago

This fixes a case where email errors were sent but application/json content-type was not set.

This changes nothing for the caller.

cbenz commented 7 years ago

@laem @fpagnoux Review please :-) I let you couple of days?

laem commented 7 years ago

@cbenz I trust you on this one. But... emails ?

cbenz commented 7 years ago

Emails are sent when an exception is raised, so we can know about it.

Due to a commit quite long ago, which returns a JSON exception to the client, emails were not sent. This PR fixes it.

MattiSG commented 7 years ago

What's the use case for tracking the deployed instance and your strategy for handling the incoming email? I kinda had the impression the backlog was pretty full already…?

cbenz commented 7 years ago

I kinda have the impression there is sarcasm in here. Whatever, I maintain this is not a good practice to ignore exceptions in production.

The last PR of @fpagnoux (but not his fault!) raised some exception (see discussion) and we did know about it only when our server monitoring told me "hey legislation explorer is broken" ... by email ^^

MattiSG commented 7 years ago

No sarcasm. I'm just saying that I usually don't increase my inbound flow when I'm already drowning. I personally prefer not to spend more time sorting inbound email warning me of issues I already know than fixing said issues.

fpagnoux commented 7 years ago

If I understand correctly, the emails we are talking about are automatically generated by the server when something crashes, which should happen really rarely, right @cbenz ? Will an email by thrown if I just send a bad input to the server ?

cbenz commented 7 years ago

Exactly, these emails are only sent on uncaught exceptions, and generate HTTP 500 errors, and aren't sent when HTTP errors are returned to the user. Uncaught exceptions have not happened since many months indeed. 

Christophe Benz développeur Etalab

-------- Message d'origine -------- De : Florian Pagnoux notifications@github.com Date : 20/10/2016 12:58 (GMT+01:00) À : openfisca/openfisca-web-api openfisca-web-api@noreply.github.com Cc : Christophe Benz christophe.benz@data.gouv.fr, Mention mention@noreply.github.com Objet : Re: [openfisca/openfisca-web-api] Refactor middlewares (#70)

If I understand correctly, the emails we are talking about are automatically generated by the server when something crashes, which should happen really rarely, right @cbenz ?

Will an email by thrown if I just send a bad input to the server ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openfisca/openfisca-web-api","title":"openfisca/openfisca-web-api","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/openfisca/openfisca-web-api"}},"updates":{"snippets":[{"icon":"PERSON","message":"@fpagnoux in #70: If I understand correctly, the emails we are talking about are automatically generated by the server when something crashes, which should happen really rarely, right @cbenz ?\r\nWill an email by thrown if I just send a bad input to the server ?"}],"action":{"name":"View Pull Request","url":"https://github.com/openfisca/openfisca-web-api/pull/70#issuecomment-255074802"}}}

fpagnoux commented 7 years ago

In that case, I understand the benefit of these emails, as they allow you to be informed of unexpected and potentially harmful errors. I wonder though if better testing could have protected us from such a bug. Not sure, as it was a bug in the API due to a change in france, two independent packages.