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

Missing shadows after upgrading from 3.7.0 to 3.7.5 (and 3.8.5) #658

Closed
rreitmann opened this Issue Apr 9, 2018 · 12 comments

Comments

Projects
None yet
2 participants
@rreitmann

rreitmann commented Apr 9, 2018

We are using Javers to historize our domain objects and it is doing a great job! However, after upgrading from 3.7.0 to 3.7.5 and above the following junit tests in our spring boot project fail (the branch is configured to use javers 3.8.5 at the moment):
Missing Study Shadows
Missing Survey Shadows

Both tests use the following piece of code to load the shadows with limit=5 and skip=0 (from GenericDomainObjectVersionsService.java)

public List<T> findPreviousVersions(String id, int limit, int skip) {
    T domainObject = repository.findOne(id);

    if (domainObject == null) {
      return null;
    }

    QueryBuilder jqlQuery = QueryBuilder.byInstance(domainObject)
        .withScopeDeepPlus(Integer.MAX_VALUE).limit(limit).skip(skip);

    List<Shadow<T>> previousVersions = javers.findShadows(jqlQuery.build());

    return previousVersions.stream().map(shadow -> {
      T domainObjectVersion = shadow.get();
      if (domainObjectVersion.getId() == null) {
        // deleted shadow
        domainObjectVersion.setLastModifiedBy(shadow.getCommitMetadata().getAuthor());
        domainObjectVersion.setLastModifiedDate(shadow.getCommitMetadata().getCommitDate());
      }
      return domainObjectVersion;
    }).collect(Collectors.toList());
  }

Both tests expect to get 3 shadows (1 initial commit + 2 updates) but do get only 2 shadows.

Please let me know if this is an intended change introduced with 3.7.5 or if this is an unintended side effect from the performance improvements introduced with 3.7.5

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 10, 2018

Yes, there were significant changes in 3.7.5 in Shadow query runner (see https://javers.org/release-notes).
Remember that limit param limits the number of Snapshots and not the number of Shadows.
One Shadow can be reconstructed from many Snapshots (depending on your object graph structure).

What I can recommend, when you are testing Shadow queries, set the Snapshots limit with safe margin, for ex 100.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 10, 2018

see Performance & Profiling section in this javadoc https://javers.org/javadoc_3.x/org/javers/core/Javers.html#findShadows-org.javers.repository.jql.JqlQuery-

Please reopen this issue if you think you found a bug

@rreitmann

This comment has been minimized.

rreitmann commented Apr 10, 2018

Thanks for the clarification with the limit param. I was not aware that limit does not limit the number of shadows but the number of snapshots. Does the same count for skip? If so then I am not sure how I can quickly (performance wise) page through the shadows to present a pageable list of domain object versions to the user in which the user selects a previous version in order to restore it. Can you give me a hint?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 10, 2018

I need to check it ...

@rreitmann

This comment has been minimized.

rreitmann commented Apr 10, 2018

I think I found a way for achieving what I need. I do now page through the snapshots of the root of the object graph and collect the commit ids. Afterwards I query for all shadows which can be reconstructed for the collected commit ids. I am assuming that there will always be a snapshot for the root even if only a property further down in the graph is changed.

 public List<T> findPreviousVersions(String id, int limit, int skip) {
    QueryBuilder snapshotQuery = QueryBuilder.byInstanceId(id, domainObjectClass)
        .limit(limit).skip(skip);

    List<CdoSnapshot> snapshots = javers.findSnapshots(snapshotQuery.build());
    if (snapshots.isEmpty()) {
      return new ArrayList<>();
    }
    
    List<BigDecimal> commitIds = snapshots.stream().map(
        snapshot -> snapshot.getCommitId().valueAsNumber())
        .collect(Collectors.toList());
    
    List<Shadow<T>> previousVersions = javers.findShadows(
        QueryBuilder.byInstanceId(id, domainObjectClass)
        .withCommitIds(commitIds).build());

    return previousVersions.stream().map(shadow -> {
      T domainObjectVersion = shadow.get();
      if (domainObjectVersion.getId() == null) {
        // deleted shadow
        domainObjectVersion.setLastModifiedBy(shadow.getCommitMetadata().getAuthor());
        domainObjectVersion.setLastModifiedDate(shadow.getCommitMetadata().getCommitDate());
      }
      return domainObjectVersion;
    }).collect(Collectors.toList());
  }

Is that correct?

@rreitmann

This comment has been minimized.

rreitmann commented Apr 13, 2018

@bartoszwalacik Can you please check if my assumption above is correct.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 13, 2018

@rreitmann seems correct but could be slow. I would be better to have correct & fast paging for Shadows in JaVers itself. Let me check it. I'll be back to you.

bartoszwalacik added a commit that referenced this issue Apr 22, 2018

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 22, 2018

Ok, so seems like we should fix limit() and skip() for Shadow queries. The failing test is pushed.

bartoszwalacik added a commit that referenced this issue May 11, 2018

#658
initial work
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 23, 2018

fixed in 3.10.0

Use the new Stream API for Shadow queries — javers.findShadowsAndStream() with Stream.skip() and Stream.limit(). This is the only correct way for paging Shadows.

@rreitmann

This comment has been minimized.

rreitmann commented Jul 4, 2018

@bartoszwalacik I have just upgraded to the new API and it works like a charm 😃 One remark:
I had to call Stream.skip() and Stream.limit() in the right order: shadows.skip(skip).limit(limit);

The other way arround did not work when I page to the second page!

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jul 9, 2018

Thanks @rreitmann, yes in Java Streams ordering is important.

@rreitmann

This comment has been minimized.

rreitmann commented Jul 9, 2018

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