Closed Bug 1336452 Opened 8 years ago Closed 7 years ago

make the admin api more consistent and easier to work with

Categories

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: ashish.sareen95)

References

Details

(Whiteboard: [lang=python][gsoc])

Balrog's admin api is mostly hand written code, with some usage of wtforms for validation. It largely works, but it has some rough spots and inconsistencies. A few examples: - The logic for handling changes to Releases is needlessly complex (https://github.com/mozilla/balrog/blob/fac5aee1003d7a13810cf2b7fbdd7127921637da/auslib/admin/views/releases.py#L33), and we still find subtle bugs in it. - The Forms we use have a ton of repetition in them (https://github.com/mozilla/balrog/blob/fac5aee1003d7a13810cf2b7fbdd7127921637da/auslib/admin/views/forms.py), which I think can be reduced. - We only do minimal validation of Form arguments, and don't always provide good error messages. - We don't have a complete API client - Response formats are not consistent across all endpoints, especially when it comes to errors We should be able to fix much of the above by switching to Swagger based API. This will also have some additional benefits: - Less code to maintain (we'll be maintaining swagger specs instead, though). - Ability to generate clients for pretty much any language. - Much, much better autodocs for the API spec. One big question here is whether or not we can or should make a drop-in replacement for the current API and iterate from there. We'd make things easier on clients by doing that, but may lose some of the benefits. If we go the route of designing a new API, we should put a bit more forethought into it this time! Eg: think about use cases like l10n updates where it may be beneficial to make many changes a part of one request, rather than multiple requests. There's probably other UI or automation things where a different API would make things easier, too.
Priority: -- → P2
It might be good if we could look up things like maximum column sizes in the database, rather than duplicate it in the web layer. I'm not sure if this will be possible to do in the Swagger files, but it's worth looking into.
Priority: P2 → P3
Whiteboard: [lang=python] → [lang=python][gsoc]
Commit pushed to master at https://github.com/mozilla/balrog https://github.com/mozilla/balrog/commit/d6dc856d68919c155316ef132a73b4e57389a12c Bug 1336452 : Migrating /rules , /users and /csrf_token to connexion app (#312). r=bhearsum
Assignee: nobody → ashish.sareen95
Depends on: 1365669
Commit pushed to master at https://github.com/mozilla/balrog https://github.com/mozilla/balrog/commit/e17108a644c21e337d2801615aef4095f29bb4bb Bug 1336452: migrate all remaining Rules and Users APIs (#317). r=bhearsum
Depends on: 1369043
Depends on: 1369528
Ashish, unfortunately I had to back out https://github.com/mozilla/balrog/commit/e17108a644c21e337d2801615aef4095f29bb4bb due to some bustage with the API. Requests to /rules/:rule_id were failing with errors like: Caught HTTPError: { "detail": "None is not of type 'string'", "status": 400, "title": "Bad Request", "type": "about:blank" } After printing out the entire exception from BalrogRequestBodyValidator, I found this error in the backend logs: balrogadmin_1 | Failed validating 'type' in schema['allOf'][0]: balrogadmin_1 | {'description': 'CSRF Token in POST/PUT or UPDATE requests', balrogadmin_1 | 'properties': {'csrf_token': {'example': '1491342563##c4e6fef0b978e6c89af9ff1015e67b9ca7c45d14', balrogadmin_1 | 'type': 'string'}}, balrogadmin_1 | 'required': ['csrf_token'], balrogadmin_1 | 'title': 'CSRF Token', balrogadmin_1 | 'type': 'object'} balrogadmin_1 | balrogadmin_1 | On instance: balrogadmin_1 | None It seems like something is wrong with the parameters description for this (and maybe other) endpoints. You should be able to reproduce the problem locally with: $ curl -X POST -F 'data_version=55' -F 'csrf_token=1496356832##1416ea42807fa5206f9861016afe9459ccc6929e' -F 'mapping=Thunderbird-54.0b3-build' -L 'http://localhost:8080/api/rules/60' { "detail": "None is not of type 'object'", "status": 400, "title": "Bad Request", "type": "about:blank" }
That's weird. I'm unable to reproduce the error. Also the basic authentication and content-type headers are missing in the curl request. Swagger UI generates this request : curl -X POST "http://localhost:8080/api/rules/60" -H "accept: application/json" -H "content-type: application/json" -H "authorization: Basic YmFscm9nYWRtaW46YmFscm9nYWRtaW4=" -d "{ \"csrf_token\": \"1496376297##694f072eed5a2ec16f9f214f93745e741ab7bff9\", \"mapping\": \"Thunderbird-54.0b1-build3\", \"data_version\": 45}" Tested it out. Work correctly without any error. Here's the logs : https://pastebin.mozilla.org/9023334
Depends on: 1369781
We did some more investigation and it turns out that the client tools submit as form data (not json), and swagger doesn't allow us to accept both for the same endpoint. I've filed bug 1369781 to switch the client to json.
(In reply to Ben Hearsum (:bhearsum) from comment #6) > We did some more investigation and it turns out that the client tools submit > as form data (not json), and swagger doesn't allow us to accept both for the > same endpoint. I've filed bug 1369781 to switch the client to json. I expect this to be fixed late next week, after which I can re-land the backed out PR.
Commit pushed to master at https://github.com/mozilla/balrog https://github.com/mozilla/balrog/commit/e511474cf473018d69de265b4b5dc0537e12dab8 Bug 1336452: migrate all remaining Rules and Users APIs (#317). r=bhearsum
Commit pushed to master at https://github.com/mozilla/balrog https://github.com/mozilla/balrog/commit/41fa1be7eabc7455b39d79708bda26c124eee8d1 Bug 1336452 : Specific users view + Release History Diffs & View API migration (#323). r=bhearsum
Depends on: 1374991
Commit pushed to master at https://github.com/mozilla/balrog https://github.com/mozilla/balrog/commit/426127c3a1108eb90b0da94972fd3d80e1044682 Bug 1336452: convert Required Signoffs APIs to swagger (#342). r=bhearsum
Depends on: 1378423
Commit pushed to master at https://github.com/mozilla/balrog https://github.com/mozilla/balrog/commit/8da1568e25bd5050da29a717c62a2a6defc5ff4f Bug 1336452: Scheduled Changes First group of APIs (#355). r=bhearsum
Depends on: 1390197
The final two parts of this landed in production today. If there's any follow-ups needed we'll track them in new bugs. Thank you so much for all of your hard work on this, Ashish.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Release Engineering → Release Engineering Graveyard
You need to log in before you can comment on or make changes to this bug.