QueryBuilder.limit() should not be used with findShadowsAndStream() - as it's misleading

[tolsam]

When using the findShadowsAndStream method and making a query with limit set to a number smaller then the actual object's shadows then the method returns incorrect number of shadows.

See test case

[IlyaNerd]

@bartoszwalacik seems like this is a documented behavior, org.javers.core.Javers#findShadowsAndStream, and limit is used as a fetch size and not as an actual limit, i would propose to restrict using limit this way, just like .skip(), as its confusing and add a reasonable fetch size by default and mb a property or a param in query builder for customization

[bartoszwalacik]

true, there is no bug, limit() works as it shoud and as documented.
How it's confusing? The is the default limit (100)

 <li>{@link Javers#findShadowsAndStream(JqlQuery)} &mdash;
     *   the resulting stream is <b>lazily loaded</b> and it's limited only by
     *   the size of your JaversRepository and your heap.
     *   When the limit is hit, Javers repeats a given query to load a next bunch of Snapshots.
     *   Shadow graphs loaded by findShadowsAndStream() are always complete,
     *   but can trigger a lot of queries.
     *  

class ShadowStreamLimitBug extends Specification {

    def "should find shadows and stream with limit" () {
        given:
        Javers javers = JaversBuilder.javers().build()
        Employee frodo = new Employee("Frodo")
        frodo.addSubordinate(new Employee("Sam"))

        javers.commit("author", frodo)

        when:
        (1..10).forEach( i -> {
            frodo.setSalary(1_000 * i)
            javers.commit("author", frodo)
        })
        Stream<Shadow<Employee>> shadows = javers.findShadowsAndStream(
                QueryBuilder.byInstanceId("Frodo", Employee.class).limit(2).build())

        then:
        shadows.count() == 11
    }
}

[IlyaNerd]

confusing in terms of naming when we use limit as not a limit, but as a fetch size for paging.
i mean this issue exists because of confusion)

[bartoszwalacik]

Agreed, but this confusion is not easy to solve. For all find*() methods except findShadowsAndStream - limit() means the real limit :)
Any suggestions?

[IlyaNerd]

suggestion is the same - restrict using limit for stream method, just like its done for .skip - org.javers.repository.jql.ShadowStreamQueryRunner#queryForShadowsStream if (query.getQueryParams().skip() > 0) { throw new JaversException(JaversExceptionCode.MALFORMED_JQL, "skip can't be set on a JqlStreamQuery. Use Stream.skip() on a resulting Stream."); }
but then, i guess, this will break existing code which relies on this confusing naming, so we should provide a replacement option - either by properties or QueryBuilder setting or...

[bartoszwalacik]

new semantic of limit() in Shadow queries (both List ans Stream)

Paging & limit
Use QueryBuilder#skip(int) and QueryBuilder#limit(int) for paging
But remember that to create one Shadow, Javers typically needs to load more than
one Snapshot.
When QueryBuilder#snapshotQueryLimit(int) is hit, Javers repeats a given query
to load next bunch of Shadows until the limit set by QueryBuilder#limit(int) is reached.

Returned list of Shadow graphs is always complete (according to the selected ShadowScope
but the whole operation can trigger a few DB queries.

[bartoszwalacik]

fixed in 6.0.0-rc1

see https://javers.org/documentation/jql-examples/#limit-filter