Ticket #2741 (closed defect: fixed)

Opened 9 months ago

Last modified 7 months ago

MetaItem.__cmp__ is faulty

Reported by: mithrandi Assigned to: mithrandi
Priority: highest Milestone:
Component: Axiom Severity: normal
Keywords: Cc:
Estimated Completion (YYYY/MM/DD): Branch: branches/metaitem-cmp-2741
Author: mithrandi

Description

Different schema versions of the same item type compare equal, which breaks the upgrade system (among other things).

Change History

10/07/08 19:32:11 changed by mithrandi

  • branch set to branches/metaitem-cmp-2741.
  • author set to mithrandi.

(In [16792]) Branching to 'metaitem-cmp-2741'

10/07/08 19:33:34 changed by mithrandi

  • keywords set to review.
  • owner changed from mithrandi to exarkun.
  • priority changed from high to highest.

10/07/08 19:37:08 changed by mithrandi

The symptoms of the upgrade breakage are that any items that are newer than the oldest schema version present, but not fully upgraded, will be ignored by the automatic upgrade system, although retrieving the item directly via getItemByID or reference attribute will cause the item to be upgraded correctly.

10/07/08 20:16:01 changed by glyph

  • owner changed from exarkun to glyph.
  • status changed from new to assigned.

10/07/08 20:47:59 changed by exarkun

It might be good to have a test for the behavior which was wrong which applications directly care about (ie, the upgrade system failing). It might also be good to have a more comprehensive set of tests for the __cmp__ implementation, along the lines of http://divmod.org/exponent-trac/browser/trunk/Blendix/blendix/test/test_util.py#L44 or http://twistedmatrix.com/trac/browser/trunk/twisted/python/test/test_util.py#L499

10/07/08 20:48:24 changed by exarkun

Oh, also note #2639, of which this is at least partially a duplicate.

10/07/08 21:39:57 changed by mithrandi

I've added two tests (one white-boxish, one black-boxish) that directly cover the upgrade system failure, and there are already some more comprehensive tests for __cmp__.

10/14/08 12:30:57 changed by exarkun

  • owner changed from glyph to exarkun.
  • status changed from assigned to new.

10/14/08 12:31:01 changed by exarkun

  • status changed from new to assigned.

10/14/08 12:58:48 changed by exarkun

  • keywords deleted.
  • owner changed from exarkun to mithrandi.
  • status changed from assigned to new.

axiom.test.test_upgrading.SubStoreCompat.test_multipleLegacyVersions is failing, presumably because SubStoreCompat overrides openStore and doesn't set currentStore.

The docstring for StoreUpgradeTests.test_multipleLegacyVersions could be a bit more specific. It's not really testing the code that enqueues types for upgrade, but the code that manages the queue.

The cmp-calling line in MetaItem.__cmp__ is wider than 80 cols now.

10/14/08 13:02:31 changed by mithrandi

(In [16839]) Don't use self.currentStore refs #2741

10/14/08 13:05:17 changed by mithrandi

(In [16840]) Wrap to 80 columns. Refs #2741

10/14/08 13:07:10 changed by mithrandi

  • keywords set to review.
  • owner changed from mithrandi to exarkun.

I've addressed the first and third points. It seems to me that StoreUpgradeTests.test_multipleLegacyVersions is in fact testing the code that enqueues types for upgrade, as well as the code that manages the queue; the specific bug that I'm fixing here only affects the management code, but doesn't the test cover the whole process?

10/15/08 10:35:46 changed by exarkun

  • keywords deleted.
  • owner changed from exarkun to mithrandi.

It seems to me that StoreUpgradeTests?.test_multipleLegacyVersions is in fact testing the code that enqueues types for upgrade, as well as the code that manages the queue; the specific bug that I'm fixing here only affects the management code, but doesn't the test cover the whole process?

StoreUpgradeTests.test_multipleLegacyVersions doesn't cover the whole process. I wonder if I understand what you mean here, since that seems like a wildly inaccurate statement to me. :) To me, its docstring suggests that it's testing the behavior in Store._startup, because it's that method which actually goes and enqueues a bunch of type upgrades. That method isn't covered at all by StoreUpgradeTests.test_multipleLegacyVersions. Maybe I'm misinterpreting the test method docstring as well?

10/21/08 10:36:19 changed by mithrandi

  • keywords set to review.
  • owner changed from mithrandi to exarkun.

As per IRC conversation, I thought you were talking about a different test. I've renamed this test to avoid confusion, and rewritten the docstring; hopefully the new one is a bit better.

11/04/08 10:42:22 changed by exarkun

  • keywords deleted.
  • owner changed from exarkun to mithrandi.

Looks good, please merge.

11/04/08 11:17:21 changed by mithrandi

  • status changed from new to closed.
  • resolution set to fixed.

(In [16922]) Fix MetaItem?.cmp

Fixes #2741 Author: mithrandi Reviewer: exarkun

Different schema versions of the same item type now no longer compare equal, which fixes some breakage in the upgrade system, among other things.

12/03/08 14:33:04 changed by mithrandi

(In [16895]) Rename the test and rewrite the docstring. Refs #2741

jethro@divmod.org