Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#2377 - Wrong order in batch save - Fix to transaction depth #2378

Merged
merged 4 commits into from
Sep 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions ebean-core/src/main/java/io/ebeaninternal/api/SpiTransaction.java
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public interface SpiTransaction extends Transaction {
Boolean getBatchGetGeneratedKeys();

/**
* Modify and return the current 'depth' of the transaction.
* Modify the current 'depth' of the transaction.
* <p>
* As we cascade save or delete we traverse the object graph tree. Going up
* to Assoc Ones the depth decreases and going down to Assoc Manys the depth
Expand All @@ -139,12 +139,31 @@ public interface SpiTransaction extends Transaction {
* The depth is used for ordering batching statements. The lowest depth get
* executed first during save.
*/
void depth(int diff);
default void depth(int diff) {
// do nothing
}

/**
* Decrement the depth BUT only if depth is greater than 0.
* Return the amount that depth should be incremented by (0 or 1).
*/
default void depthDecrement() {
// do nothing
}

/**
* Reset the depth back to 0 - done at the end of top level insert and update.
*/
default void depthReset() {
// do nothing
}

/**
* Return the current depth.
*/
int depth();
default int depth() {
return 0;
}

/**
* Return true if dirty beans are automatically persisted.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,16 @@ public void depth(int diff) {
transaction.depth(diff);
}

@Override
public void depthDecrement() {
transaction.depthDecrement();
}

@Override
public void depthReset() {
transaction.depthReset();
}

@Override
public int depth() {
return transaction.depth();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ public enum Type {
this.label = label;
}

/**
* Reset the transaction depth back to 0.
*/
public void resetDepth() {
transaction.depthReset();
}

@Override
public void addTimingBatch(long startNanos, int size) {
// nothing by default
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,6 @@ private BatchedBeanHolder getBeanHolder(PersistRequestBean<?> request) {

int depth = transaction.depth();
BeanDescriptor<?> desc = request.descriptor();

// batching by bean type AND depth
String key = desc.rootName() + ":" + depth;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ public <T> List<T> publish(Query<T> query, Transaction transaction) {
} else {
update(request);
}
request.resetDepth();
}

draftHandler.updateDrafts(transaction, mgr);
Expand Down Expand Up @@ -417,7 +418,7 @@ public void update(EntityBean entityBean, Transaction t) {
} else {
update(req);
}

req.resetDepth();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reset the depth after each top level insert and update. This means that all our top level requests at depth zero are treated in the order that they are saved().

req.commitTransIfRequired();
req.flushBatchOnCascade();

Expand Down Expand Up @@ -455,6 +456,7 @@ public void insert(EntityBean bean, Transaction t) {
try {
req.initTransIfRequiredWithBatchCascade();
insert(req);
req.resetDepth();
req.commitTransIfRequired();
req.flushBatchOnCascade();

Expand Down Expand Up @@ -1123,7 +1125,7 @@ private void saveAssocOne(PersistRequestBean<?> request) {
&& !prop.isReference(detailBean)
&& !request.isParent(detailBean)) {
SpiTransaction t = request.transaction();
t.depth(-1);
t.depthDecrement();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic here - effectively we only decrement the depth when it is already greater than 0. This has the impact that it now will not make the depth negative (and hence order it before other persists requests at depth zero).

When we want to do this we now need to ensure we reset the depth back to zero after top level inserts and updates occur.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is it OK to increment the depth two lines below, if we did not decrement it?

int decremented = t.depthDecrement(); // returns 0 or 1
saveRecurse(...)
t.depth(decremented )

May also other places where depth is modified be affected by this kind of bug (e.g. deleteAssocMany, deleteList ...)
t.depth(decremented);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is it OK to increment the depth two lines below, if we did not decrement it?

Yes I think so. I actually started off with code like what you have (I called decremented delta) but actually that just left things at depth 0 and was ok but I felt it was better to instead ~ "move everything down 1" in terms of depth.

deleteAssocMany, deleteList ...

I am not sure there is the same ordering issue with deletes yet. I'd prefer to have a failing test first.

saveRecurse(detailBean, t, null, request.flags());
t.depth(+1);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,18 +232,6 @@ public boolean isSaveAssocManyIntersection(String intersectionTable, String bean
throw new IllegalStateException(notExpectedMessage);
}

@Override
public void depth(int diff) {
}

/**
* Return the current depth.
*/
@Override
public int depth() {
return 0;
}

@Override
public void markNotQueryOnly() {
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -484,34 +484,28 @@ public final boolean isSaveAssocManyIntersection(String intersectionTable, Strin
return existingBean.equals(beanName);
}

/**
* Return the depth of the current persist request plus the diff. This has the
* effect of changing the current depth and returning the new value. Pass
* diff=0 to return the current depth.
* <p>
* The depth of 0 is for the initial persist request. It is modified as the
* cascading of the save or delete traverses to the the associated Ones (-1)
* and associated Manys (+1).
* </p>
* <p>
* The depth is used to help the ordering of batched statements.
* </p>
*
* @param diff the amount to add or subtract from the depth.
*/
@Override
public final void depth(int diff) {
depth += diff;
}

/**
* Return the current depth.
*/
@Override
public final int depth() {
return depth;
}

@Override
public final void depthDecrement() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this effectively means it won't take the depth negative. So relative to other independent persist requests also at depth zero will execute in the order that they are persisted.

Fixes testMultiSave4() and testMultiSave4_usingRelation4()

if (depth != 0) {
depth += -1;
}
}

@Override
public final void depthReset() {
depth = 0;
}

@Override
public final void markNotQueryOnly() {
this.queryOnly = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,15 +310,6 @@ public Boolean getBatchGetGeneratedKeys() {
return null;
}

@Override
public void depth(int diff) {
}

@Override
public int depth() {
return 0;
}

@Override
public boolean isExplicit() {
return false;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package io.ebeaninternal.server.transaction;

import io.ebean.*;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.tests.model.basic.relates.*;

class TransactionTest extends BaseTestCase {

private final Relation1 r1 = new Relation1("R1");
private final Relation2 r2 = new Relation2("R2");
private final Relation3 r3 = new Relation3("R3");

private Transaction txn;

@BeforeEach
void beginTransaction() {
txn = DB.beginTransaction();
txn.setBatchMode(true);
}

@AfterEach
void commitTransaction() {
txn.commit();
txn.close();
}

@Test
void testMultiSave1() {
r2.setWithCascade(r3);
r1.setWithCascade(r2);
DB.save(r1);
}

@Test
void testMultiSave2() {
r2.setWithCascade(r3);
r1.setWithCascade(r2);
DB.save(r3);
DB.save(r1);
}

@Test
void testMultiSave3() {
r2.setWithCascade(r3);
r1.setWithCascade(r2);
DB.save(r3);
DB.save(r2);
DB.save(r1);
}

@Test
void testMultiSave4() {
r2.setNoCascade(r3);
r1.setWithCascade(r2);
DB.save(r3);
// Workaround: txn.flush();
DB.save(r1);
}

@Test
void testMultiSave4_usingRelation4() {
Relation4 r4 = new Relation4("foo");
r2.setR4NoCascade(r4);
r1.setWithCascade(r2);
DB.save(r4);
DB.save(r1);
}

@Test
void testMultiSave5() {
r2.setNoCascade(r3);
r1.setNoCascade(r2);
DB.save(r3);
DB.save(r2);
DB.save(r1);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package org.tests.model.basic.relates;


import java.util.UUID;

import javax.persistence.*;

import io.ebean.annotation.ChangeLog;

/**
* Relation entity
*/
@Entity
@ChangeLog
public class Relation1 {

@Id
private UUID id = UUID.randomUUID();

private String name;

public Relation1(String name) {
this.name = name;
}

@ManyToOne
private Relation2 noCascade;

@ManyToOne(cascade = CascadeType.ALL)
private Relation2 withCascade;

public UUID getId() {
return id;
}

public void setId(UUID id) {
this.id = id;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}

public Relation2 getNoCascade() {
return noCascade;
}

public void setNoCascade(Relation2 noCascade) {
this.noCascade = noCascade;
}

public Relation2 getWithCascade() {
return withCascade;
}

public void setWithCascade(Relation2 withCascade) {
this.withCascade = withCascade;
}

}
Loading