Deleting a milestone != default milestone

[ahaferburg@…]

We ended up deleting one of our milestones. The tickets are now displayed as milestone "(empty)". Judging from the SQL their milestone is NULL. New tickets, however, where the milestone dropdown box is left at the default value, are filed under "Milestone ", and I guess this means that the milestone is the empty string, "".

That means new tickets end up in a different milestone than the tickets from the deleted milestone, and I don't think there is any way to change the milestone from NULL to "".

[Christian Boos]

Reproduced.

Right, it seems better to be consistent here. However, looking at the code (blame), it seems it was done on purpose in r7581. So it looks like the inconsistency is due to a change from NULL to "" when setting the milestone to "None" in the ticket editor.

That means new tickets end up in a different milestone than the tickets from the deleted milestone, and I don't think there is any way to change the milestone from NULL to "".

Changing it to any other milestone, then back again to the empty entry does the trick. But as mentioned above, better leave them to NULL until we solve that for new tickets.

I hesitate a bit for the milestone. It seems fine to fix this for 1.0.2 but 1.1.2 would give us the possibility to "fix" the "" values during an upgrade.

[Christian Boos]

As mentioned elsewhere, we have the empty value to help us not introduce spurious diffs with '' vs None, so it seems indeed fine to fix that for 1.0.2 without touching to the '' values already in the db.

[Ryan J Ollos]

I'll see about working up a patch for this one as well.

[Ryan J Ollos]

I struggled to figure out where any negative effects of the NULL vs empty string are seen. Eventually I found a difference in Report 3.

Does the problem manifest itself anywhere else in Trac? I imagine this ambiguity could cause a number of unforeseeable problems for plugins and various other customizations as well.

The issue seems to be fixed already for ticket properties retrieved through a Ticket object (which calls Ticket._fetch_ticket​). In that case, the milestone property (and other ticket fields) will always be returned as an empty string, regardless of whether the database value is an empty string or null. However, a report such as Report 3 that runs a direct SQL query will continue to see differences between the empty string and null in the database.

I looked at 4 cases:

1. Creating a ticket when the milestone table is empty. Ticket.insert iterates over the fields and sets the values in the database, however since the milestone table is empty the field is treated as if it didn't exist​. The milestone is therefore left as null.
2. Creating a ticket when the milestone table is not empty. The default milestone is the empty string.
3. Creating a ticket when all of the milestones are closed. The milestone is forced to the default milestone and therefore the empty string is set.
4. When a milestone is deleted or closed and the tickets are retargetted to the None milestone. When the milestone is deleted and tickets retargetted to None, the milestone will be set to null after [7581]. However, if the milestone is closed and ticket retargetted to None, the milestone will be set to the empty string.

It looks to me like this only needs to be fixed at the database level since _fetch_ticket will already replace nulls with empty strings when retrieving ticket values. We just need to consistently store the data using whatever is chosen to represent the empty value. Due to (1), using the empty string as the empty value in the database would be a bit difficult. Since unset values in the database will remain null, it will be much easier to obtain consistency in the database by replacing empty strings with None before inserting into the ticket table. This might be accomplished by a patch as simple as the following (but needs more testing first):

trac/ticket/model.py

@@ -238 +238 @@
             cursor.execute("INSERT INTO ticket (%s) VALUES (%s)"
                            % (','.join(std_fields),
                               ','.join(['%s'] * len(std_fields))),
-                           [values[name] for name in std_fields])
+                           [values[name] or None for name in std_fields])
             tkt_id = db.get_last_id(cursor, 'ticket')
 
             # Insert custom fields
@@ -246 +246 @@
                 db.executemany(
                     """INSERT INTO ticket_custom (ticket, name, value)
                        VALUES (%s, %s, %s)
-                    """, [(tkt_id, c, self[c]) for c in custom_fields])
+                    """, [(tkt_id, c, self[c] or None) for c in custom_fields])
 
         self.id = tkt_id
         self.resource = self.resource(id=tkt_id)

So a possible solution is to do the following on 1.1.x:

• Apply the patch above.
• Create an upgrade script to replace existing empty string values in the database with null.

[Ryan J Ollos]

I'm moving this forward since I'm not sure what to do about comment:5 yet. I'll continue working the ticket if I get some input on the direction to take, or become more confident on deciding which direction to take.

[loomchild]

Your solution works, it will keep running the patched version and report any potential problems.

[loomchild]

I see one problem.

0. I have a ticket with assigned milestone, let's say 'M1'.
1. I modify the ticket milestone and set it to empty string in dropdown list.
2. I go to Active Tickets by Milestone report and again I see a single ticket in a new section "Milestone" in addition to (empty). This ticket's milestone field is set to an empty string instead of NULL.

[Ryan J Ollos]

I overlooked the need for modifications to Ticket.save_changes, in addition to the changes made to Ticket.insert. t11018.patch​ should fix the issue described in comment:9.

We'll also need to decide if empty strings should be replaced with nulls in the ticket_change table. I haven't made any modifications to ticket_change yet.

[loomchild <pub@…>]

Great, patch works. Thanks for working on this.

As for the table I manually replace empty strings with nulls and I haven't encountered any issues yet. I will keep testing.

[Ryan J Ollos]

Looks like it might be better to add a _to_db_types method and do the '' → None replacement of values in the dictionary in a single location. Ticket._to_db_types(...) was already added on trunk in [11331#file1].

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11018 address the issue for the Ticket model. Work in #11419 is leading to some significant refactoring that will fix the issue for the other model classes of the Ticket system. In #11419 I'll propose an upgrade step that will selectively replace empty strings with NULLs in the database.

[Ryan J Ollos]

Looking at [11331#file1], log:rjollos.git:t11018 should replace db_values[name] with db_values.get(name).

[Ryan J Ollos]

Committed to 1.0-stable in [13546], merged to trunk in [13547].

[Ryan J Ollos]

The issue reported in #11891 was discovered while working on this ticket.

[Ryan J Ollos]

TracDev/ScratchPad/DataModels@4#Standardizemissingvalues was updated to account for changes made in this ticket.

[Ryan J Ollos]

Replying to Ryan J Ollos:

Committed to 1.0-stable in [13546], merged to trunk in [13547].

Fixed empty time field not stored as NULL in r17016, merged in r17017.

[Jun Omae]

Replying to Ryan J Ollos:

Fixed empty time field not stored as NULL in r17016, merged in r17017.

After the changes, stored value of ticket_custom has been changed. We should add unit tests for the behavior.

Trac 1.2.4

$ git checkout trac-1.2.4
HEAD is now at db32919e8 1.2.4: Release Trac 1.2.4
$ make clean Trac.egg-info start-python
>>> from trac.ticket.model import Ticket
>>> from trac.test import EnvironmentStub
>>> env = EnvironmentStub()
>>> env.config.set('ticket-custom', 'time1', 'time')
>>> env.config.save()
>>> t = Ticket(env)
>>> t['status'] = 'new'
>>> t['summary'] = 'Summ'
>>> t['time1'] = None
>>> t.insert()
1
>>> env.db_query('SELECT * FROM ticket_custom WHERE ticket=1')
[(1, u'time1', u'')]    # <==

After r17016

$ git checkout mirror/1.2-stable
HEAD is now at 59edb8709 1.2.5dev: Fix time custom field not stored as NULL
$ make clean Trac.egg-info start-python
>>> from trac.ticket.model import Ticket
>>> from trac.test import EnvironmentStub
>>> env = EnvironmentStub()
>>> env.config.set('ticket-custom', 'time1', 'time')
>>> env.config.save()
>>> t = Ticket(env)
>>> t['status'] = 'new'
>>> t['summary'] = 'Summ'
>>> t['time1'] = None
>>> t.insert()
1
>>> env.db_query('SELECT * FROM ticket_custom WHERE ticket=1')
[(1, u'time1', None)]    # <==

[Ryan J Ollos]

Replying to Jun Omae:

After the changes, stored value of ticket_custom has been changed.

That is the intention.

We should add unit tests for the behavior.

r17016 has test coverage.

$ git diff
diff --git a/trac/ticket/model.py b/trac/ticket/model.py
index f3dcd40e8..d1f847370 100644
--- a/trac/ticket/model.py
+++ b/trac/ticket/model.py
@@ -59,7 +59,7 @@ def _db_str_to_datetime(value):

 def _datetime_to_db_str(dt, is_custom_field):
     if not dt:
-        return None
+        return ''
     ts = to_utimestamp(dt)
     if is_custom_field:
         # Padding with '0' would be easy to sort in report page for a user
$ make test=trac/ticket/tests/model.py
[...]
python  setup.py -q test -s trac.ticket.tests.model.test_suite
..F.......F....................................................................................
======================================================================
FAIL: test_change_empty_strings_stored_as_null (trac.ticket.tests.model.TicketTestCase)
Ticket fields with empty strings are NULL when changing ticket.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/tests/model.py", line 233, in test_change_empty_strings_stored_as_null
    self._test_empty_strings_stored_as_null(ticket)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/tests/model.py", line 207, in _test_empty_strings_stored_as_null
    [ticket.id] + cst_fields))
AssertionError: Lists differ: [(None,), (None,), (None,), (N... != [(None,), (None,), (None,), (u...

First differing element 3:
(None,)
(u'',)

- [(None,), (None,), (None,), (None,)]
?                              ^^^^

+ [(None,), (None,), (None,), (u'',)]
?                              ^^^


======================================================================
FAIL: test_create_empty_strings_stored_as_null (trac.ticket.tests.model.TicketTestCase)
Ticket fields with empty strings are NULL when creating ticket.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/tests/model.py", line 221, in test_create_empty_strings_stored_as_null
    self._test_empty_strings_stored_as_null(ticket)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/tests/model.py", line 207, in _test_empty_strings_stored_as_null
    [ticket.id] + cst_fields))
AssertionError: Lists differ: [(None,), (None,), (None,), (N... != [(None,), (None,), (None,), (u...

First differing element 3:
(None,)
(u'',)

- [(None,), (None,), (None,), (None,)]
?                              ^^^^

+ [(None,), (None,), (None,), (u'',)]
?                              ^^^


----------------------------------------------------------------------
Ran 95 tests in 0.852s

FAILED (failures=2)
Test failed: <unittest.runner.TextTestResult run=95 errors=0 failures=2>
error: Test failed: <unittest.runner.TextTestResult run=95 errors=0 failures=2>
make: *** [all] Error 1