Issue Details (XML | Word | Printable)

Key: GLASSFISH-699
Type: Bug Bug
Status: Open Open
Priority: Minor Minor
Assignee: mf125085
Reporter: craig_mcc
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
glassfish

Incorrect handling of EntityManager.remove()

Created: 01/Jun/06 09:07 PM   Updated: 06/Mar/12 09:56 PM
Component/s: entity-persistence
Affects Version/s: 9.0pe
Fix Version/s: not determined

Time Tracking:
Not Specified

Environment:

Operating System: All
Platform: All


Issuezilla Id: 699
Status Whiteboard:

MEDIUM

Tags:
Participants: craig_mcc, gfbugbridge, marina vatkina, mf125085, Mitesh Meswani, mrjdo, pkrogh, Tom Mueller and tware


 Description  « Hide

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.



Tom Mueller added a comment - 06/Mar/12 09:56 PM

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


pkrogh added a comment - 08/Mar/07 11:52 AM

Reprioritized based on BUG triage. P4 MEDIUM.


tware added a comment - 13/Sep/06 09:00 AM

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.

mrjdo added a comment - 11/Sep/06 06:10 PM

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.


craig_mcc added a comment - 09/Sep/06 09:00 PM

> 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.


mf125085 added a comment - 08/Sep/06 03:04 PM

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.


craig_mcc added a comment - 06/Sep/06 08:09 PM

> 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).


mf125085 added a comment - 06/Sep/06 04:40 PM

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.


marina vatkina added a comment - 11/Aug/06 03:42 PM

Reassigned to Markus


pkrogh added a comment - 25/Jul/06 12:25 PM

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.


gfbugbridge added a comment - 26/Jun/06 09:36 AM

<BT6443430>


Mitesh Meswani added a comment - 02/Jun/06 02:04 PM

Reassigning...