action #64075
closedUse validation consistently in WebAPI controllers
0%
Description
While working on client scripts I ran into bugs to do with incorrect use of the API and error handling. So I thought it worth investigating how we currently validate parameters and what could be done to make it more predictable and more reliable.
Retroactively filing this ticket to track my investigation and proposals.
Checklist
- JobTemplates create,update
- JobGroup create,update
- Asset register,get,delete
- Bug list,show,create,update,destroy
- Comment create
- Job list
- Locks
- Worker list,create
Updated by livdywan about 4 years ago
- Status changed from New to In Progress
I chose the JobTemplates controller to add validation in gh#os-autoinst/openQA#2781 to test out my proofs of concept:
- Mojolicious::Validation
- Detect unknown parameters
- Consider typing of API routes
Updated by livdywan about 4 years ago
Validation on the JobGroups controller is revealing bigger gaps in test coverage: gh#os-autoinst/openQA#2812
Updated by mkittler about 4 years ago
- Related to coordination #64322: [epic] Improve feedback on multi-machine API errors added
Updated by livdywan about 4 years ago
cdywan wrote:
I chose the JobTemplates controller to add validation in gh#os-autoinst/openQA#2781 to test out my proofs of concept:
- Mojolicious::Validation
- Detect unknown parameters
- Consider typing of API routes
- Distinguish missing from invalid parameters
Updated by livdywan about 4 years ago
Updated by livdywan about 4 years ago
And Bugs is also getting the validation treatment and a refreshes on the unit tests: https://github.com/os-autoinst/openQA/pull/2873
Updated by okurz about 4 years ago
PR has been merged 14 days ago, no problems reported. I assume by now also some users have done enough interaction with the API to not find problems with this. Do you see further places where work needs to be done or are we done here?
Updated by livdywan about 4 years ago
- Checklist item changed from to [x] JobTemplates create,update, [x] JobGroup create,update, [x] Asset register,get,delete, [x] Bug list,show,create,update,destroy, [ ] Comment create, [ ] Job list, [x] Locks, [ ] Mm get_children,get_parents, [ ] Worker list,create
- Subject changed from Use validation consistently in controllers to Use validation consistently in WebAPI controllers
To make it clearer what's needed to resolve this, I'm adding a checklist of what's been implemented and what probably should be implemented here.
Updated by livdywan about 4 years ago
- Checklist item changed from [x] JobTemplates create,update, [x] JobGroup create,update, [x] Asset register,get,delete, [x] Bug list,show,create,update,destroy, [ ] Comment create, [ ] Job list, [x] Locks, [ ] Mm get_children,get_parents, [ ] Worker list,create to [x] JobTemplates create,update, [x] JobGroup create,update, [x] Asset register,get,delete, [x] Bug list,show,create,update,destroy, [ ] Comment create, [x] Job list, [x] Locks, [ ] Mm get_children,get_parents, [ ] Worker list,create
Updated by livdywan about 4 years ago
- Checklist item changed from [x] JobTemplates create,update, [x] JobGroup create,update, [x] Asset register,get,delete, [x] Bug list,show,create,update,destroy, [ ] Comment create, [x] Job list, [x] Locks, [ ] Mm get_children,get_parents, [ ] Worker list,create to [x] JobTemplates create,update, [x] JobGroup create,update, [x] Asset register,get,delete, [x] Bug list,show,create,update,destroy, [x] Comment create, [x] Job list, [x] Locks, [x] Worker list,create
Updated by livdywan about 4 years ago
Semi-related, verifying test logs of the validation PRs, I noticed
- test coverage doesn't cover error logging in
lib/OpenQA/WebAPI/Controller/Running.pm - we have some not so helpful backtrace spew in
lib/OpenQA/Schema/Result/Jobs.pmwhich would benefit from getting being reduced to the error at hand like we do elsewhere
Updated by livdywan almost 4 years ago
Another semi-related PR https://github.com/os-autoinst/openQA/pull/3021
Updated by livdywan almost 4 years ago
Updated by livdywan almost 4 years ago
- Status changed from In Progress to Feedback
Updated by livdywan almost 4 years ago
- Status changed from Feedback to Resolved
Updated by AdamWill over 3 years ago
The validation for the 'latest' param for querying jobs seems weirdly strict, since the code simply checks whether it's set to anything at all. I was using 'latest=true' in some code in the Python client, and that has broken on update to recent openQA git because now only 'latest=1' is valid.