Opened 10 years ago
Last modified 4 years ago
#11419 closed enhancement
More user-friendly error message from trac-admin console commands when IntegrityError in database — at Version 25
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: |
|
||
API Changes: |
Added |
||
Internal Changes: |
Description (last modified by )
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.
Change History (25)
comment:2 by , 9 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:3 by , 9 years ago
Milestone: | 1.0.3 → 1.1.3 |
---|
comment:4 by , 9 years ago
Milestone: | 1.1.3 → 1.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 , 9 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 , 9 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 , 9 years ago
Seems that Milestone
class has the same issue, which is introduced in [9692].
comment:8 by , 9 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.
comment:9 by , 9 years ago
Release Notes: | modified (diff) |
---|
comment:10 by , 9 years ago
Milestone: | 1.1.4 → 1.1.5 |
---|
follow-ups: 15 18 21 comment:11 by , 9 years ago
In [bf7afd8be/rjollos.git], I don't think we should raise TracError
s rather than IntegrityError
s from AbstractEnum
classes. After the changes, unique key violations couldn't be handled by plugins. Also, I've written a plugin which traps IntegrityError
s and shows warnings in admin panels for Trac 0.12 using IRequestFilter
.
My suggestions:
- For
IntegrityError
s: use new exception likeResourceNotFound
, e.g.ResourceDuplicated
inherited fromTracError
. - For invalid inputs: use new exception like
ValueError
, e.g.TracValueError
inherited fromTracError
.
comment:12 by , 9 years ago
Milestone: | 1.1.5 → 1.1.6 |
---|
comment:13 by , 9 years ago
Milestone: | 1.1.6 → 1.1.7 |
---|
follow-up: 17 comment:15 by , 9 years ago
Replying to jomae:
ResourceDuplicated
inherited fromTracError
.
How about ResourceExistsError
or ResourceNotUniqueError
?
comment:16 by , 9 years ago
Description: | modified (diff) |
---|
comment:17 by , 9 years ago
Replying to rjollos:
How about
ResourceExistsError
orResourceNotUniqueError
?
+1 for ResourceExistsError
.
follow-up: 19 comment:18 by , 9 years ago
Replying to Jun Omae:
- For
IntegrityError
s: use new exception likeResourceNotFound
, e.g.ResourceDuplicated
inherited fromTracError
.
Do you need ResourceExistsError
to inherit from IntegrityError
? That's appears to be a bit tricky because of the database specific exceptions.
comment:19 by , 9 years ago
Replying to Jun Omae:
- For
IntegrityError
s: use new exception likeResourceNotFound
, e.g.ResourceDuplicated
inherited fromTracError
.Do you need
ResourceExistsError
to inherit fromIntegrityError
? 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 , 8 years ago
Milestone: | 1.2 → 1.3.1 |
---|
follow-up: 24 comment:21 by , 8 years ago
Replying to Jun Omae:
- For invalid inputs: use new exception like
ValueError
, e.g.TracValueError
inherited fromTracError
.
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 , 8 years ago
DONE Update TracDev/Exceptions with ResourceExistsError
and TracValueError
classes.
comment:24 by , 7 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 , 7 years ago
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.
Sorry, I've misunderstood. Please Ignore it.
Trac 1.0 and 1.0.1 don't show backstrace of the errors.