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

Delete with no revisions present throws #461

Closed
gmadkat opened this Issue Nov 16, 2016 · 26 comments

Comments

Projects
None yet
3 participants
@gmadkat

gmadkat commented Nov 16, 2016

I am using JaVers in my Spring application, and when I am trying to delete an entity which has no revision history, I am seeing this exception: JaversException CANT_DELETE_OBJECT_NOT_FOUND: failed to delete object , it doesn't exists in JaversRepository
I looked at your code and noticed that is is intentional, do you have a workaround or would you consider allowing a way for us to delete entities without revisions? I am able to do a PUT (update) on an entity without revision history though, just not a delete.

Code I think this is happening in:

public Commit createTerminalByGlobalId(String author, Map<String, String> properties, GlobalId removedId){
       Validate.argumentsAreNotNull(author, properties, removedId);
       Optional<CdoSnapshot> previousSnapshot = javersRepository.getLatest(removedId);
       if (previousSnapshot.isEmpty()){
           throw new JaversException(JaversExceptionCode.CANT_DELETE_OBJECT_NOT_FOUND,removedId.value());
       }
       CommitMetadata commitMetadata = nextCommit(author, properties);
       CdoSnapshot terminalSnapshot = snapshotFactory.createTerminal(removedId, previousSnapshot.get(), commitMetadata);
       Diff diff = diffFactory.singleTerminal(removedId, commitMetadata);
       return new Commit(commitMetadata, Lists.asList(terminalSnapshot), diff);
   }

We do need to support historical data with no revision history and delete them as needed so please help us do this.

Thank you

@gmadkat

This comment has been minimized.

gmadkat commented Nov 16, 2016

I am happy to make any changes and submit a pull request if you are ok accepting them.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 16, 2016

And these deletes of non existing objects are called by aspects (@JaversSpringDataAuditable) ?

@gmadkat

This comment has been minimized.

gmadkat commented Nov 16, 2016

Yes. We have a Postgres database of legacy data with the JpaRepository objects annotated with @JaversSpringDataAuditable, and we were trying to delete them.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 16, 2016

Ok, in this case it makes sense to swallow and log the exception. We will accept a PR, please implement this catch in OnDeleteAuditChangeHandler (in aspects impl).

@gmadkat

This comment has been minimized.

gmadkat commented Nov 16, 2016

In order to fix, I am trying to build JaVers using gradlew build on Mingw Windows and got an error:

org.javers.core.json.JsonConverterUtilTypesTest > should convert class java.io.File (\tmp\file.txt) to and from JSON FAILED
    Condition not satisfied:

    javers.jsonConverter.toJson( givenValue ) == expectedJson
    |      |             |       |            |  |
    |      |             |       \tmp\file.txt|  "/tmp/file.txt"
    |      |             "\\tmp\\file.txt"    false
    |      |                                  4 differences (76% similarity)
    |      |                                  "(\\\\)tmp(\\\\)file.txt"
    |      |                                  "(/~-~)tmp(/~-~)file.txt"
    |      org.javers.core.json.JsonConverter@696d6bb8
    org.javers.core.JaversCore@41548f5
        at org.javers.core.json.JsonConverterUtilTypesTest.should convert #expectedType (#givenValue) to and from JSON(JsonConverterUtilTypesTest.groovy:19)

821 tests completed, 1 failed
:javers-core:test FAILED

FAILURE: Build failed with an exception.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 17, 2016

what is Mingw Windows? do you use gradle normally or is it your first time?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 17, 2016

I'm not a Windows user but I think you don't need Mingw to run Gradle (Gradle is a Java app) simply run gradlew.bat

@gmadkat

This comment has been minimized.

gmadkat commented Nov 17, 2016

I am seeing errors on building on Windows 10 regardless of Mingw or a command prompt. I can attach a log. I went to Mingw in the hope that a Linux'y environment would help. I can attach a log. I can build on a Mac too and will try that, but our corporate environment is windows.

@mwesolowski

This comment has been minimized.

Member

mwesolowski commented Nov 17, 2016

@gmadkat You're right. I've reproduced this error. Currently one of our tests assumes that it is executed in UNIX environment and fails in Windows. We will fix this.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 18, 2016

@gmadkat, try latest master, @mwesolowski has just pushed a fix for Windows

@gmadkat

This comment has been minimized.

gmadkat commented Nov 18, 2016

I just tried it, I can build on Windows 10 now, thanks so much!
Gowri

@gmadkat

This comment has been minimized.

gmadkat commented Nov 28, 2016

I am implementing swallowing the exception in OnDeleteAuditChangeHandler and I am running into one problem. The throwing of the JaversException in the code below is setting the rollback flag and forcing my transaction to not commit and I cannot recover from this. Please advise on how to implement this so that a lack of history does not set the rollback flag in my transaction?

One idea I have is to not throw an exception f there is no previous snapshot rather than throw and swallow it. I suggest implement this option.

public Commit createTerminalByGlobalId(String author, Map<String, String> properties, GlobalId removedId){
        Validate.argumentsAreNotNull(author, properties, removedId);
        Optional<CdoSnapshot> previousSnapshot = javersRepository.getLatest(removedId);
        if (previousSnapshot.isEmpty()){
            throw new JaversException(JaversExceptionCode.CANT_DELETE_OBJECT_NOT_FOUND,removedId.value());
        }
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 29, 2016

This is a good idea. So instead of swallowing the exception we will check if given object exists and call javers.commitShallowDelete() only if given object exists

@gmadkat

This comment has been minimized.

gmadkat commented Nov 29, 2016

Thank you, I tried to do this and I am running into issues that I am not sure how to fix.. eg.
I need to pass in a JaversRepository to check if the object exists.
And I do think this will break some tests, is this something you would be more comfortable fixing than me fixing it?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Nov 29, 2016

ok, I will take a look

@gmadkat

This comment has been minimized.

gmadkat commented Nov 29, 2016

I actually tried the below and it appeared to fix the bug, but I do not like returning null, and i noticed that the returned commit is not used anyways in OnDeleteAuditChangeHandler

Please let me know if the below patch is acceptable, I will clean out the commented code if so and submit.

$ git diff
diff --git a/javers-core/src/main/java/org/javers/core/JaversCore.java b/javers-core/src/main/java/org/javers/core/JaversCore.java
index 6b2b3d9..97715ee 100644
--- a/javers-core/src/main/java/org/javers/core/JaversCore.java
+++ b/javers-core/src/main/java/org/javers/core/JaversCore.java
@@ -92,7 +92,7 @@ class JaversCore implements Javers {
         argumentsAreNotNull(author, properties, deleted);

         Commit commit = commitFactory.createTerminal(author, properties, deleted);
-
+        if (commit == null) return null;
         repository.persist(commit);
         logger.info(commit.toString());
         return commit;
@@ -109,6 +109,7 @@ class JaversCore implements Javers {
         argumentsAreNotNull(author, properties, globalId);

         Commit commit = commitFactory.createTerminalByGlobalId(author, properties, globalIdFactory.createFromDto(globalId));
+        if (commit == null) return null;

         repository.persist(commit);
         logger.info(commit.toString());
diff --git a/javers-core/src/main/java/org/javers/core/commit/CommitFactory.java b/javers-core/src/main/java/org/javers/core/commit/CommitFactory.java
index 77a4116..3122c2d 100644
--- a/javers-core/src/main/java/org/javers/core/commit/CommitFactory.java
+++ b/javers-core/src/main/java/org/javers/core/commit/CommitFactory.java
@@ -47,7 +47,7 @@ public class CommitFactory {
         Validate.argumentsAreNotNull(author, properties, removedId);
         Optional<CdoSnapshot> previousSnapshot = javersRepository.getLatest(removedId);
         if (previousSnapshot.isEmpty()){
-            throw new JaversException(JaversExceptionCode.CANT_DELETE_OBJECT_NOT_FOUND,removedId.value());
+           return null; // throw new JaversException(JaversExceptionCode.CANT_DELETE_OBJECT_NOT_FOUND,removedId.value());
         }
         CommitMetadata commitMetadata = nextCommit(author, properties);
         CdoSnapshot terminalSnapshot = snapshotFactory.createTerminal(removedId, previousSnapshot.get(), commitMetadata);
diff --git a/javers-core/src/test/groovy/org/javers/core/JaversCommitE2ETest.groovy b/javers-core/src/test/groovy/org/javers/core/JaversCommitE2ETest.groovy
index 664d34e..8f9e0ad 100644
--- a/javers-core/src/test/groovy/org/javers/core/JaversCommitE2ETest.groovy
+++ b/javers-core/src/test/groovy/org/javers/core/JaversCommitE2ETest.groovy
@@ -94,7 +94,7 @@ class JaversCommitE2ETest extends Specification {
                           { j -> j.commitShallowDeleteById("some.login", instanceId(1,SnapshotEntity))} ]
         opType << ["using object instance","using globalId"]
     }
-
+/*
     def "should fail when deleting non existing object"() {
         given:
         def javers = javers().build()
@@ -107,7 +107,7 @@ class JaversCommitE2ETest extends Specification {
         JaversException exception = thrown()
         exception.code == JaversExceptionCode.CANT_DELETE_OBJECT_NOT_FOUND
     }
-
+*/
     def "should create initial commit for new objects"() {
         given:
         def javers = javers().build()

@gmadkat

This comment has been minimized.

gmadkat commented Dec 1, 2016

I just wanted to follow up on this issue and see whether the above makes sense and is a fix, or if you are fixing it, can you tell me a timeframe for this? We need it asap for our release. Thank you!

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 2, 2016

@gmadkat if you want to help, contribute a pull request

@gmadkat

This comment has been minimized.

gmadkat commented Dec 2, 2016

@bartoszwalacik I can do that, does my diff above look like a reasonable fix? I can clean it up and submit it in a PR , I tested it locally and it works.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 2, 2016

Well, i dont know. It's not human readable

@gmadkat

This comment has been minimized.

gmadkat commented Dec 2, 2016

@bartoszwalacik Ok let me do a pull request and can go from there.

@gmadkat

This comment has been minimized.

gmadkat commented Dec 7, 2016

@bartoszwalacik I submitted a pull request for this fix, please let me know if it looks ok

bartoszwalacik added a commit that referenced this issue Dec 7, 2016

@bartoszwalacik

This comment has been minimized.

@gmadkat

This comment has been minimized.

gmadkat commented Dec 9, 2016

Thank you, when will this be merged?

bartoszwalacik added a commit that referenced this issue Dec 9, 2016

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 9, 2016

merged, it will be released today

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Dec 9, 2016

released in 2.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment