mozilla :: #balrog

18 May 2017
14:28aksareenbhearsum: The general exceptions are all returning HTTP 400 error with response json format as {"exception" : msg} in handleGeneralExceptions (https://github.com/mozilla/balrog/blob/master/auslib/web/admin/views/base.py#L55) . However, I defined the BadRequestResponseModel in swagger
14:28aksareenspec(https://github.com/mozilla/balrog/blob/master/auslib/web/admin/swagger/api.yaml#L195) format based on connexion's problem(https://github.com/zalando/connexion/blob/master/connexion/problem.py) for 400 errors quite differently. So right now it doesn't affect the current APIs in the spec file but for upcoming APIs , only one universal exception json
14:28aksareenformat should be throughout backend and in UI in my opinion. So which one should it be used ? Also , does any UI controller access the key 'exception' in {"exception": msg} 400 response format. If yes then it should be used throughout as changing it to connexion's probelm format will break the UI once again.
14:31aksareenbhearsum: However, connexion acting as a middleware will always use the problem format if it encounters any validation exception during request processsing before passing the data onto the admin app. So that could make it trickier to enforce one single standard format if we decide to use {"exception" : msg} format too.
14:32aksareenso IMO if UI doesn't use and expect {"exception" : msg} error format for sporadic excpetions then we should go with connexion's problem format. What do you think >
14:32aksareen?
14:36bhearsumaksareen: you can check how often the UI uses 'exception' but looking in ui/app/controllers
14:36bhearsumit's probably only a few spots
14:36bhearsumi do think it would be best to try to stick with one exception format, and i guess that has to be connexion's
14:37aksareenbhearsum: let me check the controllers first.
14:46aksareenbhearsum: greping the controller folder gave the following response below. So we can use problem format and I'll have to keep track to change these controllers to access response.detail when we migrate their APIs.
14:46aksareen$/balrog/ui/app/js/controllers$ grep "response.exception" -r ./
14:46aksareen./release_scheduled_change_delete_controller.js: text: response.exception
14:46aksareen./permission_scheduled_change_delete_controller.js: text: response.exception
14:46aksareen./rule_scheduled_change_delete_controller.js: text: response.exception
14:46aksareen./rule_delete_controller.js: sweetAlert(response.exception);
14:46aksareen./release_delete_controller.js: message = response.exception;
14:46aksareenHowever, does it imply that the "if" check at ,say this line, (https://github.com/mozilla/balrog/blob/master/ui/app/js/controllers/required_signoffs_modal_base_controller.js#L305) is now redundant as response will always be a json object of a specific format ?
14:48bhearsumyeah, i think so
14:48bhearsumany of those string responses are really old, i'm not sure we have anything that returns them anymore
14:49aksareenbhearsum: btw technically if I change the handleGeneralExceptions response formats then those controllers have to be changed to response.detail too else they'll no longer work.
14:50aksareenI meant not later but together within a single PR
14:50bhearsumyeah
14:50bhearsumthat's fine
14:50bhearsumconsistency is good!
14:57aksareenbhearsum: yeah. but I'm starting to think that all this migration is actually downgrading the UI forms. Isn't that a step back ?
14:57bhearsumthe fact that connexion/swagger only returns one error at a time does, yeah
14:58bhearsumit's not ideal, but i think the benefits outweigh it
14:58bhearsumi don't think that switching from exception to the connexion response format will make the ui worse in any way though?
17:25aksareenbhearsum: If I use this argument (https://github.com/zalando/connexion/blob/master/connexion/problem.py#L36) to pass the {"exception": val} dict then I can update the response problem json format by adding any extra custom fields of my own choice. This means that UI can still access the response.exception field from response object. Ergo, no UI change
17:25aksareenneeded. Just have to make sure not to populate response.detail field as exception and detail are redundant together
18:09bhearsumaksareen: oh cool!
18:09bhearsumit's nice to see that connexion is extensible like that
21:13aksareenbhearsum: I'm stuck on an issue. I replaced flask.Response ((https://github.com/aksareen/balrog/blob/600514bce55e6d6af27799eb0c8553c0b9606c25/auslib/web/admin/views/base.py#L31 ) with connexion.problem(https://github.com/zalando/connexion/blob/master/connexion/problem.py#L40) here . However, now multiple test cases are failing. Logs
21:13aksareen(https://pastebin.mozilla.org/9022133) . It says "TypeError: 'ConnexionResponse' object is not callable" . That's strange as I was able to call the method from /rules API (https://github.com/aksareen/balrog/blob/600514bce55e6d6af27799eb0c8553c0b9606c25/auslib/web/admin/views/rules.py#L63)
19 May 2017
No messages
   
Last message: 36 days and 19 hours ago