Red Hat Bugzilla – Bug 974893
by_name() methods should convert NoResultFound to something more meaningful
Last modified: 2018-02-05 19:41:31 EST
Description of problem:
The server error logs are full of entries like:
Traceback (most recent call last):
File "/usr/lib/python2.6/site-packages/bkr/server/xmlrpccontroller.py", line 54, in RPC2
response = self.process_rpc(method,params)
File "/usr/lib/python2.6/site-packages/bkr/server/xmlrpccontroller.py", line 43, in process_rpc
response = obj(*params)
File "/usr/lib/python2.6/site-packages/bkr/server/tasks.py", line 532, in to_dict
return Task.by_name(name, valid).to_dict()
File "/usr/lib/python2.6/site-packages/bkr/server/model.py", line 6317, in by_name
File "/usr/lib64/python2.6/site-packages/sqlalchemy/orm/query.py", line 1684, in one
raise orm_exc.NoResultFound("No row was found for one()")
NoResultFound: No row was found for one()
Steps to Reproduce:
1. Call the Task API to_dict() XML-RPC method with an unrecognised task name
The raw SQL Alchemy NoResultFound error appears in the logs
A more suitable exception like "Unknown task: <name>" is reported.
This should be done for all of the by_name() methods in the data model, not just Task.by_name.
Using an appropriate custom exception should also make it possible to handle these in a relatively generic fashion in the web UI layer (converting them to a 200 OK response with a suitable error message rather than 500 error).
Is there any reason why any lookup method (i.e by_id()) not be included in this?
I happened to notice lots of "by_name" instances in the logs, but yes, it would probably be applicable to other lookup mechanisms as well (since we do expose lookups by ID in a few places)
We catch the NoResultException in some of these queries and raise a ValueError with a friendly error message.
Is the proposed fix run *all* such queries in a try..catch block and have it raise a custom exception with a friendly message?
raise MyCustomClass('Something bad happened')
(In reply to Amit Saha from comment #4)
> We catch the NoResultException in some of these queries and raise a
> ValueError with a friendly error message.
> Is the proposed fix run *all* such queries in a try..catch block and have it
> raise a custom exception with a friendly message?
> For eg.
> except NoResultFound:
> raise MyCustomClass('Something bad happened')
Yes except it would be
return cls.query.filter(cls.name == name).one()
raise DatabaseLookupError('No such blah %r' % name)
That is, the try-except-raise should be in the by_name method itself (and this applies to all the other by_* lookup methods, in spite of the bug title).
Ray implemented this in one of his patches a while back but it got bogged down in deciding what the exception type and exception message should be, so we abandoned it.
I think for the exception type ,we could borrow some ideas from openstack.
Cheers, Matt Jia
If I give a non-existent pool name in cli, it will return "No row was found for one()" See below.
[wangdong@localhost tmp]$ bkr system-modify beaker-test-vm1 --pool-policy PoolBaa
/usr/lib/python2.7/site-packages/requests/packages/urllib3/connection.py:251: SecurityWarning: Certificate has no `subjectAltName`, falling back to check for a `commonName` for now. This feature is being removed by major browsers and deprecated by RFC 2818. (See https://github.com/shazow/urllib3/issues/497 for details.)
HTTP error: 400 Client Error: BAD REQUEST
No row was found for one()
I think it should tell user "There isn't a pool named <PoolBaa>"