Edgewall Software
Modify

Opened 6 years ago

Closed 3 years ago

#11419 closed enhancement (fixed)

More user-friendly error message from trac-admin console commands when IntegrityError in database

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: admin/console Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
  • Traceback is not shown when existing resource is added using TracAdmin (e.g. trac-admin $env milestone add <milestone>.
  • Cached fields returned by TicketSystem.get_ticket_fields were not updated when milestone due or completed dates were changed.
API Changes:

Added ResourceExistsError, which is raised when inserting a resource that already exists.

Description (last modified by Ryan J Ollos)

Console admin milestone add operation could report a nicer error when milestone already exists. For example,

$ trac-admin tracdev milestone add milestone1
IntegrityError: column name is not unique
16:29:47 Trac[console] ERROR: Exception in trac-admin command: 
Traceback (most recent call last):
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/admin/console.py", line 110, in onecmd
    rv = cmd.Cmd.onecmd(self, line) or 0
  File "/usr/lib/python2.7/cmd.py", line 220, in onecmd
    return self.default(line)
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/admin/console.py", line 286, in default
    return self.cmd_mgr.execute_command(*args)
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/admin/api.py", line 127, in execute_command
    return f(*fargs)
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/ticket/admin.py", line 411, in _do_add
    milestone.insert()
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/ticket/model.py", line 1042, in insert
    to_utimestamp(self.completed), self.description))
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/db/util.py", line 121, in execute
    cursor.execute(query, params)
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/db/util.py", line 65, in execute
    return self.cursor.execute(sql_escape_percent(sql), args)
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/db/sqlite_backend.py", line 78, in execute
    result = PyFormatCursor.execute(self, *args)
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/db/sqlite_backend.py", line 56, in execute
    args or [])
  File "/home/user/Workspace/t2311/teo-rjollos.git/trac/db/sqlite_backend.py", line 48, in _rollback_on_error
    return function(self, *args, **kwargs)
IntegrityError: column name is not unique

Instead, we'll aim for something like:

$ trac-admin tracdev milestone add milestone1
TracError: milestone "milestone1" already exists

The logic in tags/trac-1.1.6/trac/ticket/admin.py@:53,56#L30 may need to be modified.

Attachments (0)

Change History (26)

comment:1 by Jun Omae, 6 years ago

Sorry, I've misunderstood. Please Ignore it.

Trac 1.0 and 1.0.1 don't show backstrace of the errors.

$ ~/venv/trac/1.0/bin/trac-admin ~/var/trac/1.0 milestone add milestone1
IntegrityError: column name is not unique
$ ~/venv/trac/1.0.1/bin/trac-admin ~/var/trac/1.0 milestone add milestone1
IntegrityError: column name is not unique
Last edited 6 years ago by Jun Omae (previous) (diff)

comment:2 by Ryan J Ollos, 5 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:3 by Ryan J Ollos, 5 years ago

Milestone: 1.0.31.1.3

comment:4 by Ryan J Ollos, 5 years ago

Milestone: 1.1.31.1.4

This ticket has led to a pretty extensive refactoring that I haven't finished yet. I committed a few of the simple changes in [13621,13624,13625]. The rest will need to wait for 1.1.4.

comment:5 by Jun Omae, 5 years ago

The changes in [13621] have an issue which cached TicketSystem.fields should be reset when Version.time is updated. I think the changes should be reverted.

Trac 1.1.2:

>>> from trac import __version__
>>> print __version__
1.1.2
>>> from datetime import datetime
>>> from trac.test import EnvironmentStub
>>> from trac.ticket.api import TicketSystem
>>> from trac.ticket.model import Version
>>> from trac.util.datefmt import utc
>>> env = EnvironmentStub(default_data=True)
>>> tktsys = TicketSystem(env)
>>> [f['options'] for f in tktsys.fields if f['name'] == 'version']
[[u'2.0', u'1.0']]
>>> versions = Version.select(env)
>>> [v.name for v in versions]
[u'2.0', u'1.0']
>>> versions[0].time = datetime(2014, 1, 1, tzinfo=utc)
>>> versions[0].update()
>>> versions[1].time = datetime(2014, 2, 1, tzinfo=utc)
>>> versions[1].update()
>>> [v.name for v in Version.select(env)]
[u'1.0', u'2.0']
>>> [f['options'] for f in tktsys.fields if f['name'] == 'version']
[[u'1.0', u'2.0']]

Trac 1.1.3dev after [13621]:

>>> from trac import __version__
>>> print __version__
1.1.3dev
>>> from datetime import datetime
>>> from trac.test import EnvironmentStub
>>> from trac.ticket.api import TicketSystem
>>> from trac.ticket.model import Version
>>> from trac.util.datefmt import utc
>>> env = EnvironmentStub(default_data=True)
>>> tktsys = TicketSystem(env)
>>> [f['options'] for f in tktsys.fields if f['name'] == 'version']
[[u'2.0', u'1.0']]
>>> versions = Version.select(env)
>>> [v.name for v in versions]
[u'2.0', u'1.0']
>>> versions[0].time = datetime(2014, 1, 1, tzinfo=utc)
>>> versions[0].update()
>>> versions[1].time = datetime(2014, 2, 1, tzinfo=utc)
>>> versions[1].update()
>>> [v.name for v in Version.select(env)]
[u'1.0', u'2.0']
>>> [f['options'] for f in tktsys.fields if f['name'] == 'version']
[[u'2.0', u'1.0']]  # <== should be reset

comment:6 by Ryan J Ollos, 5 years ago

I'll add a unit test for that and revert the one-line change to the Version class. I won't revert the other changes unless you see a problem with them. Thanks for noticing.

comment:7 by Jun Omae, 5 years ago

Seems that Milestone class has the same issue, which is introduced in [9692].

comment:8 by Ryan J Ollos, 5 years ago

Thanks, that's a good find and I'll fix that issue as well. In the changes I've been preparing for this ticket I've added extensive unit test coverage, but I didn't consider the cached ordering for either Version or Milestone in the TicketSystem fields. It will be good to add unit-test coverage for those cases.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:9 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

Issues from comment:5 and comment:7 fixed in [13626], merged in [13627].

comment:10 by Ryan J Ollos, 5 years ago

Milestone: 1.1.41.1.5

comment:11 by Jun Omae, 4 years ago

In [bf7afd8be/rjollos.git], I don't think we should raise TracErrors rather than IntegrityErrors from AbstractEnum classes. After the changes, unique key violations couldn't be handled by plugins. Also, I've written a plugin which traps IntegrityErrors and shows warnings in admin panels for Trac 0.12 using IRequestFilter.

My suggestions:

  • For IntegrityErrors: use new exception like ResourceNotFound, e.g. ResourceDuplicated inherited from TracError.
  • For invalid inputs: use new exception like ValueError, e.g. TracValueError inherited from TracError.

comment:12 by Ryan J Ollos, 4 years ago

Milestone: 1.1.51.1.6

comment:13 by Ryan J Ollos, 4 years ago

Milestone: 1.1.61.1.7

comment:14 by Ryan J Ollos, 4 years ago

Milestone: 1.1.71.2

Milestone renamed

in reply to:  11 ; comment:15 by Ryan J Ollos, 4 years ago

Replying to jomae:

ResourceDuplicated inherited from TracError.

How about ResourceExistsError or ResourceNotUniqueError?

comment:16 by Ryan J Ollos, 4 years ago

Description: modified (diff)

in reply to:  15 comment:17 by Jun Omae, 4 years ago

Replying to rjollos:

How about ResourceExistsError or ResourceNotUniqueError?

+1 for ResourceExistsError.

in reply to:  11 ; comment:18 by Ryan J Ollos, 4 years ago

Replying to Jun Omae:

  • For IntegrityErrors: use new exception like ResourceNotFound, e.g. ResourceDuplicated inherited from TracError.

Do you need ResourceExistsError to inherit from IntegrityError? That's appears to be a bit tricky because of the database specific exceptions.

in reply to:  18 comment:19 by Jun Omae, 4 years ago

Replying to Jun Omae:

  • For IntegrityErrors: use new exception like ResourceNotFound, e.g. ResourceDuplicated inherited from TracError.

Do you need ResourceExistsError to inherit from IntegrityError? That's appears to be a bit tricky because of the database specific exceptions.

No. I need to use ResourceExistsError to inherit from TracError when unique key violation.

comment:20 by Ryan J Ollos, 3 years ago

Milestone: 1.21.3.1

in reply to:  11 ; comment:21 by Ryan J Ollos, 3 years ago

Replying to Jun Omae:

  • For invalid inputs: use new exception like ValueError, e.g. TracValueError inherited from TracError.

Is this representative of the use-case you have in mind for TracValueError?:

-        assert self.exists, "Cannot delete non-existent version"
+        if not self.exists:
+            raise TracValueError(_("Cannot delete non-existent version."))

comment:22 by Ryan J Ollos, 3 years ago

DONE Update TracDev/Exceptions with ResourceExistsError and TracValueError classes.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:23 by Christian Boos, 3 years ago

Milestone: 1.3.1next-dev-1.3.x

Moving to some later 1.3.x version.

in reply to:  21 comment:24 by Ryan J Ollos, 3 years ago

Replying to Ryan J Ollos:

Is this representative of the use-case you have in mind for TracValueError?:

-        assert self.exists, "Cannot delete non-existent version"
+        if not self.exists:
+            raise TracValueError(_("Cannot delete non-existent version."))

On further thought, it seems like we should raise TracError in the case that AssertionError was previously raised, for example: tags/trac-1.3.1/trac/ticket/model.py@:260#L257. I think these errors will only be encountered in the case of a programming error, and are not utilized for normal program flow.

Let me know if you see differently.

comment:25 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)
Milestone: next-dev-1.3.x1.3.2
Release Notes: modified (diff)

Proposed changes in log:rjollos.git:t11419.1. I hadn't looked at those changes in quite some time, and had planned to do more refactoring, but it's time to just finish this up for now.

comment:26 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [15774:15775].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.