Ticket Component Interface Fails to Properly Check for Existing Component

[pacopablo]

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()

[Christian Boos]

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.

[pacopablo]

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.

[Christian Boos]

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.

[Christian Boos]

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.