Opened 8 years ago

Closed 7 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.

Subtickets

Attachments (2)

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

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by ayleph

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:

@@ -129,7 +129,7 @@ def deleteuser(args):
     db = mg_globals.database
 
     user = db.User.query.filter_by(
-        username=unicode(args.username.lower())).one()
+        username=unicode(args.username.lower())).first()
     if user:
         user.delete()
         print 'The user %s has been deleted' % args.username
Last edited 8 years ago by ayleph (previous) (diff)

comment:2 Changed 8 years ago by ayleph

Owner: ayleph deleted
Status: in_progressreview

comment:3 Changed 8 years ago by ayleph

Milestone: 0.8.0
Resolution: fixed
Status: reviewclosed

comment:4 Changed 8 years ago by Elrond

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 Changed 8 years ago by Jessica Tallon

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 Changed 8 years ago by ayleph

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 Changed 8 years ago by Christopher Allan Webber

Milestone: 0.8.00.9.0

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

comment:8 Changed 7 years ago by Ben Sturmfels

Status: acceptedreview

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

Regards,
Ben

Changed 7 years ago by Ben Sturmfels

Attachment: issue_696.patch added

comment:9 Changed 7 years ago by Ben Sturmfels

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

comment:10 Changed 7 years ago by ayleph

Cc: berkerpeksag added

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

comment:11 Changed 7 years ago by berkerpeksag

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!

comment:12 in reply to:  11 Changed 7 years ago by ayleph

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 Changed 7 years ago by Ben Sturmfels

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

Changed 7 years ago by Ben Sturmfels

Attachment: issue_969v2.patch added

Conflicts resolved.

comment:14 Changed 7 years ago by Christopher Allan Webber

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.