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

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

[Jun Omae]

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

[Ryan J Ollos]

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.

[Jun Omae]

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

[Ryan J Ollos]

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.

[Jun Omae]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Jun Omae]

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.

[Ryan J Ollos]

Milestone renamed

[Ryan J Ollos]

Replying to jomae:

ResourceDuplicated inherited from TracError.

How about ResourceExistsError or ResourceNotUniqueError?

[Jun Omae]

Replying to rjollos:

How about ResourceExistsError or ResourceNotUniqueError?

+1 for ResourceExistsError.

[Ryan J Ollos]

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

[Jun Omae]

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.

[Ryan J Ollos]

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."))

[Ryan J Ollos]

DONE Update TracDev/Exceptions with ResourceExistsError and TracValueError classes.

[Christian Boos]

Moving to some later 1.3.x version.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

Committed to trunk in [15774:15775].