JDO
  1. JDO
  2. JDO-419

StateTransitions incorrectly asserts that field access in a deleted instance will throw an exception

    Details

      Description

      The series of states marked with "read field with active datastore transaction" (element 14 of the "transitions" field of org.apache.jdo.tck.lifecycle.StateTransitions) asserts that accessing a field of a deleted instance will throw an exception. However, this is not mandated by the spec, only suggested that it might happen. Section 5.5.6 reads: "Any other access to persistent fields is not supported and might throw a JDOUserException".

      The only easy fix is to just remove the "read field with active datastore transaction" test specifications.

      1. JDO-419.patch
        2 kB
        Craig L Russell
      2. JDO-419.patch
        0.7 kB
        Marc Prud'hommeaux

        Activity

        Hide
        Craig L Russell added a comment -

        svn commit -m "JDO-419 JDO-424 Changed state transition table to use UNSPECIFIED where the specification is unclear" trunk/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java branches/2.0.1/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java
        Sending branches/2.0.1/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java
        Sending trunk/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java
        Transmitting file data ..
        Committed revision 454538.

        Show
        Craig L Russell added a comment - svn commit -m " JDO-419 JDO-424 Changed state transition table to use UNSPECIFIED where the specification is unclear" trunk/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java branches/2.0.1/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java Sending branches/2.0.1/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java Sending trunk/tck20/src/java/org/apache/jdo/tck/lifecycle/StateTransitions.java Transmitting file data .. Committed revision 454538.
        Hide
        Michael Bouschen added a comment -

        The patch looks good!

        Show
        Michael Bouschen added a comment - The patch looks good!
        Hide
        Craig L Russell added a comment -

        Please review this patch.

        This patch uses a different flag, UNSPECIFIED, to bypass running the test. The semantic difference is that UNSPECIFIED means that the behavior is not specified, while IMPOSSIBLE means that the conditions for the test cannot be created.

        This allows us to update the life cycle table in the specification to be consistent.

        Show
        Craig L Russell added a comment - Please review this patch. This patch uses a different flag, UNSPECIFIED, to bypass running the test. The semantic difference is that UNSPECIFIED means that the behavior is not specified, while IMPOSSIBLE means that the conditions for the test cannot be created. This allows us to update the life cycle table in the specification to be consistent.
        Hide
        Ilan Kirsh added a comment -

        Hi Craig,

        Your last comment is very interesting. I didn't know that this is the new policy. Actually ObjectDB 2.0 supports dual mode - enhancement when available (but with no binary compatibility) and reflection as a backup for persistence capable classes that are not enhanced.

        Currently, out of the 445 TCK tests that ObjectDB passes - 6 tests fail in reflection mode:

        1. IsTransactionalFalse.testIsTransactionalFalse (access in line 86 is not detected)
        2. MakeTransactionalPriorToTransactionRolledback.testTransactionalInst (modifications in lines 115, 117 are not detected)
        3. InstanceLifecycleListenerLoad.testLoad (access in line 101 is not detected)
        4. NoAccessToFieldsAfterPredelete.test (access in line 115, for instance, is not detected)
        5. StateTransitions - as discussed in this issue.
        6. WhenNontransactionalReadIsFalse.test (access in line 116, for instance, is not detected)

        My plan was to offer the reflection mode as a non compatible extension, but if JDO can support reflection mode and these tests can be fixed it could be much better.

        Regards, Ilan

        Show
        Ilan Kirsh added a comment - Hi Craig, Your last comment is very interesting. I didn't know that this is the new policy. Actually ObjectDB 2.0 supports dual mode - enhancement when available (but with no binary compatibility) and reflection as a backup for persistence capable classes that are not enhanced. Currently, out of the 445 TCK tests that ObjectDB passes - 6 tests fail in reflection mode: 1. IsTransactionalFalse.testIsTransactionalFalse (access in line 86 is not detected) 2. MakeTransactionalPriorToTransactionRolledback.testTransactionalInst (modifications in lines 115, 117 are not detected) 3. InstanceLifecycleListenerLoad.testLoad (access in line 101 is not detected) 4. NoAccessToFieldsAfterPredelete.test (access in line 115, for instance, is not detected) 5. StateTransitions - as discussed in this issue. 6. WhenNontransactionalReadIsFalse.test (access in line 116, for instance, is not detected) My plan was to offer the reflection mode as a non compatible extension, but if JDO can support reflection mode and these tests can be fixed it could be much better. Regards, Ilan
        Hide
        Craig L Russell added a comment -

        In JDO 1, it was a requirement to throw an exception to access a field other than the primary key field in a deleted instance. We changed it in JDO 2 to accommodate implementations that didn't get control on each field access (reflective implementations).

        Show
        Craig L Russell added a comment - In JDO 1, it was a requirement to throw an exception to access a field other than the primary key field in a deleted instance. We changed it in JDO 2 to accommodate implementations that didn't get control on each field access (reflective implementations).
        Hide
        Marc Prud'hommeaux added a comment -

        My opinion is that it is useful to be left up to the vendor to decide whether to throw an exception or not (or only throw an exception if the field is hollow).

        Show
        Marc Prud'hommeaux added a comment - My opinion is that it is useful to be left up to the vendor to decide whether to throw an exception or not (or only throw an exception if the field is hollow).
        Hide
        Craig L Russell added a comment -

        The patch is an elegant solution to the issue.

        The only other option is to require an exception to be thrown in case the implementation is BinaryCompatible, which would require a specification change. This would also require a more extensive change to the test. I'm not sure that it's worth it.

        Other opinions?

        Show
        Craig L Russell added a comment - The patch is an elegant solution to the issue. The only other option is to require an exception to be thrown in case the implementation is BinaryCompatible, which would require a specification change. This would also require a more extensive change to the test. I'm not sure that it's worth it. Other opinions?
        Hide
        Marc Prud'hommeaux added a comment -

        Attached is a patch to fix the incorrect state transition assertions.

        Show
        Marc Prud'hommeaux added a comment - Attached is a patch to fix the incorrect state transition assertions.
        Hide
        Marc Prud'hommeaux added a comment -

        This also affects the "read field with active optimistic transaction" state list. Another way to fix it is to change elements 7 & 8 from ERROR to IMPOSSIBLE in both the "read field with active optimistic transaction" and "read field with active datastore transaction" arrays.

        Show
        Marc Prud'hommeaux added a comment - This also affects the "read field with active optimistic transaction" state list. Another way to fix it is to change elements 7 & 8 from ERROR to IMPOSSIBLE in both the "read field with active optimistic transaction" and "read field with active datastore transaction" arrays.

          People

          • Assignee:
            Craig L Russell
            Reporter:
            Marc Prud'hommeaux
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development