Cayenne
  1. Cayenne
  2. CAY-512

Keep DataObjects in commited state for no-op writeProperties

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.2 branch
    • Fix Version/s: Undefined future
    • Component/s: Core Library
    • Labels:
      None
    • Environment:
      M9-B1

      Description

      Regression: Cayenne M9+ sets DataObjects as "modified" during no-op writeProperties

      writeProperty("x", "y") puts dataObject into a modified state when "y" was already the value of "x" before the method call.

      Before M9, object would remain in committed state.

      Object in my ObjectStore:

      <ObjectId:Announcement, ANNOUNCEMENT_ID=200>=

      {<ObjectId:Announcement, ANNOUNCEMENT_ID=200>; modified; [enabled=>Y; description=>SNAP; contentList=>(..); effectiveEndDate=>Mon Oct 31 00:00:00 EST 2005; effectiveStartDate=>Thu Sep 01 00:00:00 EDT 2005; qualifiedViewpointList=>(..)]}

      ObjectDiff of record – appears to indicate that nothing has changed.
      Same as the other 4 restored objects.

      Comparision of snapshot values to object store values shows nothing different.

      value= ObjectDiff (id=3310)
      arcSnapshot= HashMap (id=3311)
      currentArcSnapshot= null
      diffId= 1
      flatIds= null
      nodeId= ObjectId (id=3309)
      objectStore= ObjectStore (id=3200)
      otherDiffs= null
      snapshot= HashMap (id=3312)

      org.objectstyle.cayenne.access.ObjectDiff@133d68a
      {}
      null
      1
      null
      <ObjectId:Announcement, ANNOUNCEMENT_ID=200>
      org.objectstyle.cayenne.access.ObjectStore@c0cf
      null

      {enabled=Y, description=SNAP, effectiveEndDate=Mon Oct 31 00:00:00 EST 2005, effectiveStartDate=Thu Sep 01 00:00:00 EDT 2005}

      A workaround has been to add code in my BaseDataObject class to check
      for the old value equaling the new value in writeProperty(), and if
      so, do nothing.

        Activity

        Hide
        Andrey Razumovsky added a comment -

        Hi Øyvind,

        The javadoc in hasChanges() says:
        "Returns <code>true</code> if there are any modified, deleted or new objects registered with this DataContext".

        So it works correct, provided "artist.setName(artist.getName());" pushes object to modified state. The problem is that isNoop() can work slowly, so we likely don't want to fire it always when hasChanges() is called.

        A new method, however, would be useful

        Show
        Andrey Razumovsky added a comment - Hi Øyvind, The javadoc in hasChanges() says: "Returns <code>true</code> if there are any modified, deleted or new objects registered with this DataContext". So it works correct, provided "artist.setName(artist.getName());" pushes object to modified state. The problem is that isNoop() can work slowly, so we likely don't want to fire it always when hasChanges() is called. A new method, however, would be useful
        Hide
        Øyvind Harboe added a comment -

        hasChanges() has problems in my case for Cayenne 3.x and I wrote the following code to work around false postivies for hasChanges().

        I think my observation belongs as a comment to this JIRA (even if I don't understant the details of the report) rather than a fresh report.

        /** DataContext.hasChanges() doesn't work because it returns true for "phantom" changes */
        static public boolean checkIfChanges(DataContext dataContext)
        {
        if (dataContext.hasChanges())

        { return !dataContext.getObjectStore().getChanges().isNoop(); }

        return false;
        }

        Show
        Øyvind Harboe added a comment - hasChanges() has problems in my case for Cayenne 3.x and I wrote the following code to work around false postivies for hasChanges(). I think my observation belongs as a comment to this JIRA (even if I don't understant the details of the report) rather than a fresh report. /** DataContext.hasChanges() doesn't work because it returns true for "phantom" changes */ static public boolean checkIfChanges(DataContext dataContext) { if (dataContext.hasChanges()) { return !dataContext.getObjectStore().getChanges().isNoop(); } return false; }
        Hide
        Mike Kienenberger added a comment -

        I agree. It's consistent between the 1.1 and 1.2 release even if it changed during the milestones, and it's too complicated for 1.2.

        Let's revisit it after 1.2 since there's a simple workaround in my case.

        Show
        Mike Kienenberger added a comment - I agree. It's consistent between the 1.1 and 1.2 release even if it changed during the milestones, and it's too complicated for 1.2. Let's revisit it after 1.2 since there's a simple workaround in my case.
        Hide
        Andrus Adamchik added a comment -

        Hmm.. all this discussion actually means that the solution is not trivial. Current diff recording is optimized - we record an object-level diff when it is touched for the first time, and then only continue to record arc diffs (to be able to replay in the right sequence for the nested contexts), but simple property diffs are not recorded one by one, saving CPU cycles.

        Also you just correctly pointed out that a check in the "writeProperty(..)" is unreliable and therefore can't be a long-term solution either.

        Reverting 'hasChanges' to the old behavior is also a partial solution that will only make things slower. Current implementation is very fast BTW, but of course does not detect phantoms:

        public synchronized boolean hasChanges()

        { return !changes.isEmpty(); }

        I guess I shouldn't be "fixing" it for 1.2 at all, as the current behavior is at least consistent - phantom mods are cleared on commit and not as a side effect of some other operation. Consistency is good In 3.0 we can make it a DataContext flag - whether to check for phantom mods immediately or defer it till commit.

        Show
        Andrus Adamchik added a comment - Hmm.. all this discussion actually means that the solution is not trivial. Current diff recording is optimized - we record an object-level diff when it is touched for the first time, and then only continue to record arc diffs (to be able to replay in the right sequence for the nested contexts), but simple property diffs are not recorded one by one, saving CPU cycles. Also you just correctly pointed out that a check in the "writeProperty(..)" is unreliable and therefore can't be a long-term solution either. Reverting 'hasChanges' to the old behavior is also a partial solution that will only make things slower. Current implementation is very fast BTW, but of course does not detect phantoms: public synchronized boolean hasChanges() { return !changes.isEmpty(); } I guess I shouldn't be "fixing" it for 1.2 at all, as the current behavior is at least consistent - phantom mods are cleared on commit and not as a side effect of some other operation. Consistency is good In 3.0 we can make it a DataContext flag - whether to check for phantom mods immediately or defer it till commit.
        Hide
        Mike Kienenberger added a comment -

        Sorry. I should have mentioned that I was detecting dirty contexts by using hasChanges(). But the problem is that those "modified" objects will not unserialize. So this is still not backwards-compatible with what I've been doing – it's just hiding the cause of the problem as my checking code now thinks there are no modifications, but unserializing will still fail on an object in a modified state!

        Again, I can fix this by doing a manual check in writeProperty().

        I hate to keep harping on the same issue, but how is it more performant to have the overhead of tracking a phantom modification than to check if the values are equal?

        In addition, tracking modifications at the application-level writeProperty() will only catch the case of setting a value twice in a row rather than performing detection of whether the object has effectively changed since last commit.

        Ie, where "a" = 1, writeProperty("a", "1") would be detected, but writeProperty("a", "2"), writeProperty("a", "1") would not.

        But this is probably outside of the scope of a 1.2 bug fix.

        Show
        Mike Kienenberger added a comment - Sorry. I should have mentioned that I was detecting dirty contexts by using hasChanges(). But the problem is that those "modified" objects will not unserialize. So this is still not backwards-compatible with what I've been doing – it's just hiding the cause of the problem as my checking code now thinks there are no modifications, but unserializing will still fail on an object in a modified state! Again, I can fix this by doing a manual check in writeProperty(). I hate to keep harping on the same issue, but how is it more performant to have the overhead of tracking a phantom modification than to check if the values are equal? In addition, tracking modifications at the application-level writeProperty() will only catch the case of setting a value twice in a row rather than performing detection of whether the object has effectively changed since last commit. Ie, where "a" = 1, writeProperty("a", "1") would be detected, but writeProperty("a", "2"), writeProperty("a", "1") would not. But this is probably outside of the scope of a 1.2 bug fix.
        Hide
        Andrus Adamchik added a comment -

        Ok, I tried the test above on the 1.1 branch and as expected phantom mods were allowed to go through.. But I think I know what the difference is between now and then - DataContext.hasChanges() now has no side effect of cleaning up phantoms (it didn't do it well in the past, but this is another story)...

        I am probably -1 on modifying writeProperty, as will add overhead for people who don't care about it (we may instead explain it in the docs how to catch phantom mods), but we may instead roll back ObjectStore.hasChanges() to its original behavior.

        Show
        Andrus Adamchik added a comment - Ok, I tried the test above on the 1.1 branch and as expected phantom mods were allowed to go through.. But I think I know what the difference is between now and then - DataContext.hasChanges() now has no side effect of cleaning up phantoms (it didn't do it well in the past, but this is another story)... I am probably -1 on modifying writeProperty, as will add overhead for people who don't care about it (we may instead explain it in the docs how to catch phantom mods), but we may instead roll back ObjectStore.hasChanges() to its original behavior.
        Hide
        Mike Kienenberger added a comment -

        By the way, if we decide this is a "pro-gression" instead of a regression, I don't personally have an issue with moving this to "After 1.2." But it will adversely affect anyone using JSF with Cayenne, which may not be in our best interests.

        What about simply detecting when the ObjectDiff arcSnapshot is empty, deleting the ObjectDiff and changing the state back to committed? Seems to me that checking for equality during writeProperty isn't enough to catch all phantom modification states, although it'd be sufficient in the case of an autowired framework like JSF pages and backing beans.

        Show
        Mike Kienenberger added a comment - By the way, if we decide this is a "pro-gression" instead of a regression, I don't personally have an issue with moving this to "After 1.2." But it will adversely affect anyone using JSF with Cayenne, which may not be in our best interests. What about simply detecting when the ObjectDiff arcSnapshot is empty, deleting the ObjectDiff and changing the state back to committed? Seems to me that checking for equality during writeProperty isn't enough to catch all phantom modification states, although it'd be sufficient in the case of an autowired framework like JSF pages and backing beans.
        Hide
        Mike Kienenberger added a comment -

        No, the only change between seeing the old behavior and the new behavior is which cayenne-nodeps.jar I drop into the classpath.

        Pre-M8, works.
        Post-M8, stacktrace.

        And I can change that to make both work but adding an explicit equality check to writeProperties.

        Furthermore, even if this was a "pro-gression" rather than a regression, I think it's a worthwhile one. Otherwise, JSF doesn't work well out of the box since you either have to hack up your own writeProperty() equality check or you can no longer directly wire your DataObjects as backing beans for the pages.

        Again, why would it be useful to track noop changes?

        Show
        Mike Kienenberger added a comment - No, the only change between seeing the old behavior and the new behavior is which cayenne-nodeps.jar I drop into the classpath. Pre-M8, works. Post-M8, stacktrace. And I can change that to make both work but adding an explicit equality check to writeProperties. Furthermore, even if this was a "pro-gression" rather than a regression, I think it's a worthwhile one. Otherwise, JSF doesn't work well out of the box since you either have to hack up your own writeProperty() equality check or you can no longer directly wire your DataObjects as backing beans for the pages. Again, why would it be useful to track noop changes?
        Hide
        Andrus Adamchik added a comment -

        I think the reason is somewhere else (a different cgen template??) ... We never checked for equality in the setter.

        Show
        Andrus Adamchik added a comment - I think the reason is somewhere else (a different cgen template??) ... We never checked for equality in the setter.
        Hide
        Mike Kienenberger added a comment -

        I can't tell you the exact versions, but from when I started using JSF (April of 2005?) until M8, the behavior was that noops didn't set the object to modified.

        With the current behavior, my datacontext is left in a dirty state because some objects appear to be modified between requests. It's only benign from a cayenne-commit standpoint, not from an application behavior standpoint, since modified objects are treated differently from committed objects.

        Show
        Mike Kienenberger added a comment - I can't tell you the exact versions, but from when I started using JSF (April of 2005?) until M8, the behavior was that noops didn't set the object to modified. With the current behavior, my datacontext is left in a dirty state because some objects appear to be modified between requests. It's only benign from a cayenne-commit standpoint, not from an application behavior standpoint, since modified objects are treated differently from committed objects.
        Hide
        Andrus Adamchik added a comment -

        for instance here is the code from 1.1.4:

        public void writeProperty(String propName, Object val) {
        resolveFault();

        // 1. retain object snapshot to allow clean changes tracking
        // 2. change object state
        if (persistenceState == PersistenceState.COMMITTED)

        { persistenceState = PersistenceState.MODIFIED; dataContext.getObjectStore().retainSnapshot(this); }

        // else....
        // other persistence states can't be changed to MODIFIED

        writePropertyDirectly(propName, val);
        }

        Show
        Andrus Adamchik added a comment - for instance here is the code from 1.1.4: public void writeProperty(String propName, Object val) { resolveFault(); // 1. retain object snapshot to allow clean changes tracking // 2. change object state if (persistenceState == PersistenceState.COMMITTED) { persistenceState = PersistenceState.MODIFIED; dataContext.getObjectStore().retainSnapshot(this); } // else.... // other persistence states can't be changed to MODIFIED writePropertyDirectly(propName, val); }
        Hide
        Andrus Adamchik added a comment -

        Mike, I can swear that CayenneDataObject.writeProperties(..) put an object in a modified state BEFORE M9:

        assertEquals(PersistenceState.COMMITTED, artist.getPersistentState());
        artist.setName(artist.getName());
        assertEquals(PersistenceState.MODIFIED, artist.getPersistentState());

        I need to try it with the old Cayenne, but AFAIK this is the default behavir that avoids unneeded equals checks. It is benign too - "phantom" modifications are cleared on commit.

        Show
        Andrus Adamchik added a comment - Mike, I can swear that CayenneDataObject.writeProperties(..) put an object in a modified state BEFORE M9: assertEquals(PersistenceState.COMMITTED, artist.getPersistentState()); artist.setName(artist.getName()); assertEquals(PersistenceState.MODIFIED, artist.getPersistentState()); I need to try it with the old Cayenne, but AFAIK this is the default behavir that avoids unneeded equals checks. It is benign too - "phantom" modifications are cleared on commit.

          People

          • Assignee:
            Andrus Adamchik
            Reporter:
            Mike Kienenberger
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development