Edgewall Software
Modify

Opened 13 years ago

Last modified 4 years ago

#4130 new defect

Ticket Component Interface Fails to Properly Check for Existing Component

Reported by: pacopablo Owned by:
Priority: normal Milestone: next-major-releases
Component: ticket system Version: 0.10
Severity: normal Keywords: model component
Cc: pacopablo@… Branch:
Release Notes:
API Changes:

Description

When using the ticket.model.Component class, it will raise a TracError if passed the name of a non-existent component on instantiation.

If the component is instantiated with an existing component name, it will assert an error if one tries to call insert(). However, if one instantiates the object with out a name, and then sets the name and owner after the fact, and then calls insert(), no checking of duplicate rows is done.

Possible borkage:

c = Component(env)
c.name = 'component1'
c.owner = 'sombody'
c.insert()

The API should be modified to either raise a TracError in the case in question, and/or provide a method for discovering whether or not a Component exist.

Personally, I think that the following would be desired behavior:

c = Component(env, 'new component')
c.insert()

Where the insert() is a noop if the component already exists. The exists property could also reflect whether or not the Component instance references an existing component or not.

c = Component(env, 'new component')
if not c.exists:
  c.insert()

I find this preferable to:

try:
  c = Component(env, 'new component')
except TracError:
  c = Component(env)
  c.name = 'new component'
  c.insert()

Attachments (0)

Change History (6)

comment:1 by Christian Boos, 13 years ago

Milestone: 0.11

I would prefer the second suggestion:

c = Component(env, 'new component')
if not c.exists:
  c.insert()

Note that the above should still raise an exception in case the same component has been created by another transaction after the exists and before the insert.

If we really don't want to have an exception in this case we could do:

c = Component(env, 'new component')
if not c.exists:
  if c.insert():
    # insert was successful

Anyway, we should converge to a consistent handling of all models.

comment:2 by pacopablo, 13 years ago

Why not simply have it noop? If it doesn't raise an exception, then that means that the component exists and/or the insert succeeded.

The only reason that I can think of for a component insert to fail are:

  1. Component already exists
  2. Some horrible db error

In the case of 1, a noop serves the same purpose. The component exists after the call to insert()

In the case of 2, we've got other issues, and catching them in the insert call probably won't help much.

comment:3 by anonymous, 13 years ago

Cc: pacopablo@… added; pacopablo@… removed

comment:4 by Christian Boos, 12 years ago

See also r5276. This fixed an error triggered by the fact that .exists didn't actually give any information about the milestone's existence in the database.

comment:5 by Christian Boos, 12 years ago

Milestone: 0.11.10.12

To sum up the proposal, we should be able to write things like:

c = Component(env, 'new component')
if not c.exists:
  c.insert()
  assert c.exists

If the c.insert() fails for some reason, it should raise a TracError exception (probably even a TracIntegrityError, see #6348).

That would make the data model code more intuitive to write (see #6808 for example), but such a change is beyond the scope of 0.11.x, I think.

comment:6 by Ryan J Ollos, 4 years ago

Owner: Jonas Borgström removed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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