glassfish
  1. glassfish
  2. GLASSFISH-699

Incorrect handling of EntityManager.remove()

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 9.0pe
    • Fix Version/s: not determined
    • Component/s: entity-persistence
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      699
    • Status Whiteboard:
      Hide

      MEDIUM

      Show
      MEDIUM

      Description

      I have been doing some experiments building JPA entity classes for tables in an
      existing database, rather than letting JPA build the schema for me.
      (Specifically, I am using the TRAVEL database that comes with Java Studio
      Creator 2 Update 1). For the purposes of this discussion, the important aspects
      are a master-detail relationship between the PERSON and TRIP tables, which is
      represented by a PERSONID column in the TRIP table that has a foreign key
      relationship with the PERSON table. This column is also NOT NULL, which is key
      to reproducing the problem issue.

      Next, I created entity classes for these tables using NetBeans 5.5 Beta. The
      relevant parts of the two classes are as follows:

      @Entity
      public class Person implements Serializable {
      ...
      @OneToMany(mappedBy="person")
      private List<Trip> trips;
      public List<Trip> getTrips()

      { return this.trips; }

      public void setTrips(List<Trip> trips)

      { this.trips = trips; }

      ...
      public void addTrip(Trip trip) {
      if ((trip == null) || (trips == null))

      { return; }
      if (!trips.contains(trip)) { trip.setPerson(this); trips.add(trip); }
      }
      public void removeTrip(Trip trip) {
      if ((trip == null) || (trips == null)) { return; }

      if (trips.contains(trip))

      { trips.remove(trip); trip.setPerson(null); }

      }
      ...
      }

      @Entity
      public class Trip implements Serializable

      { ... @JoinColumn(name="personid") @ManyToOne private Person person; ... }

      The addTrip() and removeTrip() logic was modelled after the NetBeans 5.5
      persistence example, allowing the developer to deal with the relationship solely
      through normal Java manipulations – and also to obey the JPA Spec requirement
      that the application is responsible for maintaining the validity of relationship
      variables.

      Now, in my application, I attempt to delete a Trip:

      utx.begin();
      Trip trip = ...; // The trip to be deleted
      trip = em.merge(trip); // (it was previously detached)
      trip.getPerson().removeTrip(trip);
      em.remove(trip);
      utx.commit();

      However, this causes an exception (from the commit). The underlying cause
      reported by the persistence engine is an attempt to execute the following statement:

      UPDATE TRIP SET PERSONID = NULL WHERE TRIPID = 123

      to reflect the fact that the Person property was set to null in the Trip entity.
      However, because PERSONID is a NOT NULL column in the database schema, this
      update fails. Removing the "trip.setPerson(null)" statement from the
      removeTrip() method makes the operation succeed, but appears to violate the
      requirements of Section 2.1.7 of the Persistence spec, which says:

      Note that it is the application that bears responsibility
      for maintaining the consistency of runtime relatinships –
      for example, for ensuring that the "one" and "many" sides of
      a bidirectional relationship are consistent with one another
      when the application updates the relationship.

      I believe that the updates should be skipped when it is known that the row is
      going to be deleted anyway.

      NOTE: There is a separate, but pretty bad, usability problem in how an
      application is notified of an error like this, and it seems to be common to all
      cases where the underlying SQL statement execution fails. The exception
      remported back to the application, from the utx.commit() call, is "Rollback
      Exception: Transaction has been marked for rollback" with no clue to what
      caused the problem. You have to dig in to the server log to get a handle on the
      actual problem (fortunately, the exception that is logged there is very detailed
      and helpful). But the actual problem exception should really be visible somehow
      in the exception that is returned to the application.

        Activity

        Hide
        Mitesh Meswani added a comment -

        Reassigning...

        Show
        Mitesh Meswani added a comment - Reassigning...
        Hide
        gfbugbridge added a comment -

        <BT6443430>

        Show
        gfbugbridge added a comment - <BT6443430>
        Hide
        pkrogh added a comment -

        Downgrading to P3. There are two work arounds:
        1. Don't use a Not NULL field. Granted this is not always possible. This
        will result in an extra update to a row that will ultimately be deleted.
        2. Do not set a relationship on an object that is about to be deleted.

        Show
        pkrogh added a comment - Downgrading to P3. There are two work arounds: 1. Don't use a Not NULL field. Granted this is not always possible. This will result in an extra update to a row that will ultimately be deleted. 2. Do not set a relationship on an object that is about to be deleted.
        Hide
        marina vatkina added a comment -

        Reassigned to Markus

        Show
        marina vatkina added a comment - Reassigned to Markus
        Hide
        mf125085 added a comment -

        Setting a non-nullable field to null is an error and should cause an exception.
        But we should throw a more description exception at commit time, and not rely on
        the SQLException.

        Show
        mf125085 added a comment - Setting a non-nullable field to null is an error and should cause an exception. But we should throw a more description exception at commit time, and not rely on the SQLException.
        Hide
        craig_mcc added a comment -

        > Setting a non-nullable field to null is an error and should
        > cause an exception. But we should throw a more description
        > exception at commit time, and not rely on the SQLException.

        I agree with you. So tell me again why the persistence engine is setting the
        non-nullable field to null?

        If a row is removed, any updates should be totally ignored. Anything else is a
        failure of the engine, or a total disregard for the advice that the JPA
        specification gives to applications (it is the application's responsibility to
        maintain the relationship fields in entity classes).

        Show
        craig_mcc added a comment - > Setting a non-nullable field to null is an error and should > cause an exception. But we should throw a more description > exception at commit time, and not rely on the SQLException. I agree with you. So tell me again why the persistence engine is setting the non-nullable field to null? If a row is removed, any updates should be totally ignored. Anything else is a failure of the engine, or a total disregard for the advice that the JPA specification gives to applications (it is the application's responsibility to maintain the relationship fields in entity classes).
        Hide
        mf125085 added a comment -

        Nulling out trip's person reference in

        trip.setPerson(null);

        must not be ignored, since you are explicitly telling the persistence layer to
        nullify this field. The JPA layer should tell you that you've set a non-nullable
        field to null. A way to indicate the (database-) constraint in the entity would be

        @Entity
        public class Trip implements Serializable

        { ... @JoinColumn(name="personid", nullable=false) @ManyToOne private Person person; ... }

        The exception should be a validation exception, thrown by the JPA layer rather
        than coming the database.

        Show
        mf125085 added a comment - Nulling out trip's person reference in trip.setPerson(null); must not be ignored, since you are explicitly telling the persistence layer to nullify this field. The JPA layer should tell you that you've set a non-nullable field to null. A way to indicate the (database-) constraint in the entity would be @Entity public class Trip implements Serializable { ... @JoinColumn(name="personid", nullable=false) @ManyToOne private Person person; ... } The exception should be a validation exception, thrown by the JPA layer rather than coming the database.
        Hide
        craig_mcc added a comment -

        > Nulling out trip's person reference in
        >
        > trip.setPerson(null);
        >
        > must not be ignored, since you are explicitly telling the persistence layer to
        > nullify this field. The JPA layer should tell you that you've set a non-nullable
        > field to null.

        I would agree with you **except** for the fact that the remove() operation
        will trigger a delete of this row ... so any changes you make to the column
        values are going to be thrown away anyway. Any reasonable implementation would
        optimize out the useless changes – just as you would do if you were hand coding
        the SQL calls directly.

        As it is, we are currently wasting the CPU time needed to do the useless
        updates, as well as screwing up the programming model that the JPA specification
        mandates.

        Show
        craig_mcc added a comment - > Nulling out trip's person reference in > > trip.setPerson(null); > > must not be ignored, since you are explicitly telling the persistence layer to > nullify this field. The JPA layer should tell you that you've set a non-nullable > field to null. I would agree with you ** except ** for the fact that the remove() operation will trigger a delete of this row ... so any changes you make to the column values are going to be thrown away anyway. Any reasonable implementation would optimize out the useless changes – just as you would do if you were hand coding the SQL calls directly. As it is, we are currently wasting the CPU time needed to do the useless updates, as well as screwing up the programming model that the JPA specification mandates.
        Hide
        mrjdo added a comment -

        I have to agree with Craig McClanahan.

        If you don't execute Person.removeTrip(theTrip) then at commit time, the trips collection still contains
        the Trip that you deleted. Therefore, you have to remove the trip from Person.

        And managing relationships is entirely up to the developer, so when you remove the trip from Person,
        you also have to nullify the relationship on the Trip side to avoid a model inconsistency.

        The spec currently mandates update of both sides, so I don't see a way around the user requirements to
        do exactly what Craig McClanahan (or the Netbeans code generator) has implemented.

        When you have an update to an instance (trip) that is subsequently in the same transaction going to be
        deleted, it's pretty clear that there is no point to updating any of the values in the row corresponding to
        the trip. So the implementation should silently ignore the updates of deleted instances.

        Show
        mrjdo added a comment - I have to agree with Craig McClanahan. If you don't execute Person.removeTrip(theTrip) then at commit time, the trips collection still contains the Trip that you deleted. Therefore, you have to remove the trip from Person. And managing relationships is entirely up to the developer, so when you remove the trip from Person, you also have to nullify the relationship on the Trip side to avoid a model inconsistency. The spec currently mandates update of both sides, so I don't see a way around the user requirements to do exactly what Craig McClanahan (or the Netbeans code generator) has implemented. When you have an update to an instance (trip) that is subsequently in the same transaction going to be deleted, it's pretty clear that there is no point to updating any of the values in the row corresponding to the trip. So the implementation should silently ignore the updates of deleted instances.
        Hide
        tware added a comment -

        First I'll start out by saying that I beleive optimizing out the update for
        deleted objects is a feature we should implement.

        As far as the argument about what the user is supposed to do with this kind of
        relationship, I can see both sides.

        By setting the foreign key column to not nullable in the database, you are
        saying that relationship will never be null. Setting the relationship to null
        (in the removeTrip() method) conflicts with this database setting. - You would
        expect an exception if you set that relationship to null at any other point in
        the application. The only thing different here is the fact that if you look to
        some code that will execute in the future, it negates that setting.

        From the persistence provider point of view, we have to decide what your intent
        was.

        • Did you do an update to null at some earlier point in a long running
          transaction that should have caused an exception?
        • Or did you update to null as part of your delete?

        My suggestion for addressing this issue is as follows:

        • Implement an enhancement that will optimize out the update. Make that
          functionality the default.
        • Implement a flag that allows you to turn that optimization off.
        Show
        tware added a comment - First I'll start out by saying that I beleive optimizing out the update for deleted objects is a feature we should implement. As far as the argument about what the user is supposed to do with this kind of relationship, I can see both sides. By setting the foreign key column to not nullable in the database, you are saying that relationship will never be null. Setting the relationship to null (in the removeTrip() method) conflicts with this database setting. - You would expect an exception if you set that relationship to null at any other point in the application. The only thing different here is the fact that if you look to some code that will execute in the future, it negates that setting. From the persistence provider point of view, we have to decide what your intent was. Did you do an update to null at some earlier point in a long running transaction that should have caused an exception? Or did you update to null as part of your delete? My suggestion for addressing this issue is as follows: Implement an enhancement that will optimize out the update. Make that functionality the default. Implement a flag that allows you to turn that optimization off.
        Hide
        pkrogh added a comment -

        Reprioritized based on BUG triage. P4 MEDIUM.

        Show
        pkrogh added a comment - Reprioritized based on BUG triage. P4 MEDIUM.
        Hide
        Tom Mueller added a comment -

        Bulk update to change fix version to "not determined" for all issues still open but with a fix version for a released version.

        Show
        Tom Mueller added a comment - Bulk update to change fix version to "not determined" for all issues still open but with a fix version for a released version.

          People

          • Assignee:
            mf125085
            Reporter:
            craig_mcc
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: