Opened 10 years ago

Closed 9 years ago

#969 closed defect (fixed)

gmg user commands don't fail gracefully for non existant users

Reported by: ayleph Owned by:
Priority: trivial Milestone: 0.9.0
Component: programming Keywords: gmg, delete, user, deleteuser
Cc: berkerpeksag Parent Tickets:

Description (last modified by Elrond)

Attempt to delete a non-existent user using bin/gmg deleteuser and you'll get a Traceback similar to below.

$ bin/gmg deleteuser idontexist
Traceback (most recent call last):
  File "bin/gmg", line 9, in <module>
    load_entry_point('mediagoblin==0.7.1.dev', 'console_scripts', 'gmg')()
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/__init__.py", line 124, in main_cli
    args.func(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/users.py", line 132, in deleteuser
    username=unicode(args.username.lower())).one()
  File "build/bdist.linux-x86_64/egg/sqlalchemy/orm/query.py", line 2329, in one
sqlalchemy.orm.exc.NoResultFound: No row was found for one()

Same problem exists for other commands in users.py.

Attachments (2)

issue_696.patch (3.1 KB ) - added by Ben Sturmfels 9 years ago.
issue_969v2.patch (3.1 KB ) - added by Ben Sturmfels 9 years ago.
Conflicts resolved.

Download all attachments as: .zip

Change History (16)

comment:1 by ayleph, 10 years ago

Owner: set to ayleph
Status: newin_progress

Elrond suggested the following change, which tested out fine for existing and non-existing users on my system.

mediagoblin/gmg_commands/users.py:

def deleteuser(args):
    commands_util.setup_app(args)

    db = mg_globals.database

    user = db.User.query.filter_by(
        username=unicode(args.username.lower())).one()

change to

def deleteuser(args):
    commands_util.setup_app(args)

    db = mg_globals.database

    user = db.User.query.filter_by(
        username=unicode(args.username.lower())).first()
Version 0, edited 10 years ago by ayleph (next)

comment:3 by ayleph, 10 years ago

Milestone: 0.8.0
Resolution: fixed
Status: reviewclosed

comment:4 by Elrond, 10 years ago

Description: modified (diff)
Resolution: fixed
Status: closedaccepted
Summary: gmg deleteuser doesn't fail gracefullygmg user commands don't fail gracefully for non existant users

We just realized, that users.py is affected globally.

Notes:

  1. Wait until py3 merge is over!
  2. Instead of doing unicode(...) (or six.text_type) in the db query, try doing a type=six.text_type in the .add_argument of the parser setup, so that the parser already does the typing for us.

comment:5 by Jessica Tallon, 10 years ago

Resolution: fixed
Status: acceptedclosed

I have used the type=sex.text_type as suggested above in the parser setup. The rest of this bug looks to be resolved though, current master:

(mediagoblin)[jessica@macbook mediagoblin]$ ./bin/gmg deleteuser idontexist
The user idontexist doesn't exist

As far as I can see that makes this issue closed as everything reported has been fixed. The fix I pushed is in a02de38. If I've missed anything feel free to re-open this issue or open a new issue with the specific issue.

comment:6 by ayleph, 10 years ago

Resolution: fixed
Status: closedaccepted

Originally I opened the ticket when someone pointed out that deleteuser was failing for a user that didn't exist. Elrond fixed that and merged it. Later we realized it applied to other commands as well, so we re-opened the ticket. This is still the case. The fix needs to be applied to the other commands.

For example, changepw:

$ bin/gmg changepw idontexist mynewpassword
** Message: pygobject_register_sinkfunc is deprecated (GstObject)
Traceback (most recent call last):
  File "bin/gmg", line 9, in <module>
    load_entry_point('mediagoblin==0.7.2.dev', 'console_scripts', 'gmg')()
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/__init__.py", line 156, in main_cli
    args.func(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/users.py", line 115, in changepw
    username=six.text_type(args.username.lower())).one()
  File "build/bdist.linux-x86_64/egg/sqlalchemy/orm/query.py", line 2329, in one
sqlalchemy.orm.exc.NoResultFound: No row was found for one()

And makeadmin:

$ bin/gmg makeadmin idontexist
** Message: pygobject_register_sinkfunc is deprecated (GstObject)
Traceback (most recent call last):
  File "bin/gmg", line 9, in <module>
    load_entry_point('mediagoblin==0.7.2.dev', 'console_scripts', 'gmg')()
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/__init__.py", line 156, in main_cli
    args.func(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/users.py", line 88, in makeadmin
    username=six.text_type(args.username.lower())).one()
  File "build/bdist.linux-x86_64/egg/sqlalchemy/orm/query.py", line 2329, in one
sqlalchemy.orm.exc.NoResultFound: No row was found for one()

comment:7 by Christopher Allan Webber, 10 years ago

Milestone: 0.8.00.9.0

Worth fixing, but can be bumped to next release, not a blocker for 0.8.0

comment:8 by Ben Sturmfels, 9 years ago

Status: acceptedreview

I'm attaching a patch to changepw and makeadmin to prevent these unhandled exceptions when the user doesn't exist.

Regards,
Ben

by Ben Sturmfels, 9 years ago

Attachment: issue_696.patch added

comment:9 by Ben Sturmfels, 9 years ago

I should mention that I've manually tested both valid and invalid users for changepw and makeadmin.

comment:10 by ayleph, 9 years ago

Cc: berkerpeksag added

Looks good to me, at a glance. CC'ing berker to consider pushing to master.

comment:11 by berkerpeksag, 9 years ago

I couldn't apply the patch cleanly. Could you please fix merge conflicts?

+        print(u'The user %s doesn\'t exist.' % args.username)

Can you change %s to %r? The user 'foo' doesn't exist. looks better to me. Same comment for other usages of %s.

+        type=six.text_type)

Clever! :)

Thanks!

in reply to:  11 comment:12 by ayleph, 9 years ago

I couldn't apply the patch cleanly. Could you please fix merge conflicts?

Oh, I see now that tsyesika's changes from earlier today modified this file after Sturm's patch was submitted. How exciting!

comment:13 by Ben Sturmfels, 9 years ago

Thanks for the review Berker. I've resolved the conflict in the attached patch.

by Ben Sturmfels, 9 years ago

Attachment: issue_969v2.patch added

Conflicts resolved.

comment:14 by Christopher Allan Webber, 9 years ago

Resolution: fixed
Status: reviewclosed

The patch looks good, and it didn't break any patches that aren't already broken in master (I'd better look into those!)

Great work Sturm, thanks for the patch! And thanks for your review berker, as well as your contributions on this one ayleph!

Sometimes things take a while, but they make it... :)

Note: See TracTickets for help on using tickets.