Bug 974893 - by_name() methods should convert NoResultFound to something more meaningful
by_name() methods should convert NoResultFound to something more meaningful
Status: NEW
Product: Beaker
Classification: Community
Component: web UI (Show other bugs)
0.12
Unspecified Unspecified
unspecified Severity unspecified (vote)
: ---
: ---
Assigned To: beaker-dev-list
tools-bugs
:
Depends On: 978225
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-17 00:09 EDT by Nick Coghlan
Modified: 2016-05-26 09:21 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Nick Coghlan 2013-06-17 00:09:53 EDT
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
    return query.one()
  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()


How reproducible:

Always

Steps to Reproduce:
1. Call the Task API to_dict() XML-RPC method with an unrecognised task name
2.
3.

Actual results:

The raw SQL Alchemy NoResultFound error appears in the logs

Expected results:

A more suitable exception like "Unknown task: <name>" is reported.

Additional info:

This should be done for all of the by_name() methods in the data model, not just Task.by_name.
Comment 1 Nick Coghlan 2013-06-26 02:58:03 EDT
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).
Comment 2 Raymond Mancy 2014-02-17 02:42:26 EST
Is there any reason why any lookup method (i.e by_id()) not be included in this?
Comment 3 Nick Coghlan 2014-02-17 02:45:01 EST
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)
Comment 4 Amit Saha 2014-05-05 23:41:35 EDT
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.

try:
   SomeClass.by_name('something')
except NoResultFound:
   raise MyCustomClass('Something bad happened')
Comment 5 Dan Callaghan 2014-05-06 18:48:28 EDT
(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.
> 
> try:
>    SomeClass.by_name('something')
> except NoResultFound:
>    raise MyCustomClass('Something bad happened')

Yes except it would be

try:
    return cls.query.filter(cls.name == name).one()
except NoResultFound:
    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.
Comment 6 matt jia 2014-05-09 01:07:39 EDT
I think for the exception type ,we could borrow some ideas from openstack.

http://docs.openstack.org/developer/python-openstackclient/api/openstackclient.common.exceptions.html

Cheers, Matt Jia
Comment 7 wangdong 2015-04-21 02:11:54 EDT
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.)
  SecurityWarning
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>"

Note You need to log in before you can comment on or make changes to this bug.