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

Testcase for ebean with circular dependences #1723

Merged
merged 2 commits into from
Jun 10, 2019

Conversation

tobias-
Copy link
Contributor

@tobias- tobias- commented May 23, 2019

Two failing test cases.

  1. While the javax.persistance api discourages the use of bidirectional
    cascades, it does not prohibit them. They're very useful and natural
    if you think of the database as a graph instead of a tree.
    test case 1 (testCircularCascade) tests this.
  2. Second test case fetches the child, but saves the parent. If used like
    that, the child will not have it's changes saved, even with cascade = ALL.

1. While the javax.persistance api discourages the use of bidirectional
   cascades, it does not prohibit them. They're very useful and natural
   if you think of the database as a graph instead of a tree.
   test case 1 (testCircularCascade) tests this.
2. Second test case fetches the child, but saves the parent. If used like
   that, the child will not have it's changes saved.
return convertFromTimestamp(dateTimeParser.parse(jsonDateTime));
Instant instant = Instant.parse(jsonDateTime.replace(" ", "T"));
Timestamp ts = Timestamp.from(instant);
return convertFromTimestamp(ts);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not happy with this change

  1. why do you need to replace with T? (btw when replacing single characters it is better to use replace(' ', 'T'))
  2. the change makes the dateTimeParser.parse method obsolete. So it would be better to change the 'parse' implementation.

What was the problem here? I assume, the precision of fractions of the DateTimeParser. Newer java versions (11+) probable needs up to 9 digits: `yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS'Z'
@rbygrave I think it would be a good idea to refactor the DateTimeParser, so that it uses Instant.parse instead of SimpleDateFormat

Copy link
Member

Choose a reason for hiding this comment

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

good idea to refactor the DateTimeParser, so that it uses Instant.parse instead of SimpleDateFormat

Yes, I agree @rPraml @tobias- ... Refactoring UtilDateTimeParser to use Instant.parse() looks like the good option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rPraml @rbygrave Sorry, I managed to put extra stuff on this branch (it's been force pushed away not long after @rPraml commented on this PR). The java.time issue (#1725) is PR #1724 and nothing else. This PR is about test cases for circular dependencies.

@rbygrave
Copy link
Member

Nice tests. I should get to them shortly (tomorrow night).

@tobias-
Copy link
Contributor Author

tobias- commented May 27, 2019

@rbygrave I can make these test cases work by moving the request.executeOrQueue() to before saveAssocOne(). That makes a lot of other test cases fail though. Below is what I mean.

diff --git a/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java b/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java
index 0d02a49ef..3d8134b7b 100644
--- a/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java
+++ b/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java
@@ -546,10 +546,6 @@ public final class DefaultPersister implements Persister {
     }
     request.flagUpdate();
     try {
-      if (request.isPersistCascade()) {
-        // save associated One beans recursively first
-        saveAssocOne(request);
-      }
 
       if (request.isDirty()) {
         request.executeOrQueue();
@@ -558,6 +554,11 @@ public final class DefaultPersister implements Persister {
         logger.debug(Message.msg("persist.update.skipped", request.getBean()));
       }
 
+      if (request.isPersistCascade()) {
+        // save associated One beans recursively first
+        saveAssocOne(request);
+      }
+
       if (request.isPersistCascade()) {
         // save all the beans in assocMany's after
         saveAssocMany(request);

@rbygrave rbygrave merged commit 5264464 into ebean-orm:master Jun 10, 2019
@rbygrave
Copy link
Member

Note that there is not a fix for testFetchChildModifyChildSaveParent() yet. This test just marked with @Ignore until I can look at it more.

@rbygrave rbygrave added the bug label Jun 10, 2019
@rbygrave rbygrave added this to the 11.39.2 milestone Jun 10, 2019
@tobias- tobias- deleted the circular_saves_bugs branch July 7, 2020 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants