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)
Release Engineering Graveyard
Applications: Balrog (backend)
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.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P2
Reporter | ||
Comment 1•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [lang=python] → [lang=python][gsoc]
Comment 2•7 years ago
|
||
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
Comment 3•7 years ago
|
||
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
Reporter | ||
Comment 4•7 years ago
|
||
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
Reporter | ||
Comment 6•7 years ago
|
||
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.
Reporter | ||
Comment 7•7 years ago
|
||
(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.
Comment 8•7 years ago
|
||
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
Comment 9•7 years ago
|
||
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
Comment 10•7 years ago
|
||
Commit pushed to master at https://github.com/mozilla/balrog
https://github.com/mozilla/balrog/commit/4a0670db4b96b07010b703f72388fc0345ccc53d
Bug 1336452 - Release APIs Migration (#336). r=bhearsum
Comment 11•7 years ago
|
||
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
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
Commit pushed to master at https://github.com/mozilla/balrog
https://github.com/mozilla/balrog/commit/e99b26ba54acb807ed699a63fa681447187cbcb8
Bug 1336452: Scheduled changes Remaining APIs (#368). r=bhearsum
Reporter | ||
Comment 16•7 years ago
|
||
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
Updated•5 years ago
|
Product: Release Engineering → Release Engineering Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•