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

Multiple value support for JQL commitProperty #912

Closed
antch opened this issue Nov 5, 2019 · 15 comments
Closed

Multiple value support for JQL commitProperty #912

antch opened this issue Nov 5, 2019 · 15 comments

Comments

@antch
Copy link

antch commented Nov 5, 2019

Currently there is no way to query on a commit property with multiple values using "IN" semantics, instead you have to query multiple times which is very inefficient:

Sets.newHashSet("1b2e", "8f32").stream()
    .flatMap(v -> javers.findSnapshots(QueryBuilder
        .byClass(myClass)
        .withCommitProperty("traceId", v)
        .build()).stream());

It would be much nicer to be able to simply query by multiple property values, for example:

javers.findSnapshots(QueryBuilder
    .byClass(myClass)
    .withCommitProperties("traceId", Sets.newHashSet("1b2e", "8f32"))
    .build());
@bartoszwalacik
Copy link
Member

Hi @antch , now we have AND semantic for commit props, and you need OR, right?
First, let's talk about API changes in QueryBuilder, what do you suggest?

@bartoszwalacik bartoszwalacik added contribution wanted this feature is wanted but won't be implemented by core team due to limited resources good first issue new feature labels Nov 5, 2019
@antch
Copy link
Author

antch commented Nov 5, 2019

Correct, "OR" semantics. It looks like this already exists for commit IDs via .withCommitIds(Collection<BigDecimal>), so it would probably make sense to use a similar syntax:

.withCommitProperties(String, Collection<String>)

@antch
Copy link
Author

antch commented Nov 5, 2019

This seems like a straight-forward enhancement so I could probably contribute, but unfortunately it has to be outside of work time.

@bartoszwalacik
Copy link
Member

bartoszwalacik commented Nov 5, 2019

We all here have our day jobs ...

OR/AND semantic should be expressed in methods names

.withAllCommitProperties()

.withAnyCommitProperty()

go on :)

@antch
Copy link
Author

antch commented Nov 5, 2019

Good point. What about withCommitPropertyIn(String, Collection<String>) ?

My comment about work was only to communicate that I am not allowed to contribute to OSS during work time even if used AT work. I will be happy to do the enhancement as time permits.

@bartoszwalacik
Copy link
Member

bartoszwalacik commented Nov 5, 2019

nice,
withCommitPropertyIn(String, Collection<String>) seems legit. What happens if I call it multiple times?

Please add new test case in JaversRepositoryE2ETest

@antch
Copy link
Author

antch commented Nov 5, 2019

I'd say that the last invocation wins (overwrites), to be consistent with the other builder methods. I'd also think that calling withCommitProperty and withCommitPropertyIn using the same property name would work the same way, last one called is the one that is used. I'm imagining withCommitProperty essentially delegating to withCommitPropertyIn using a single-element collection.

@bartoszwalacik
Copy link
Member

The whole builder works in this way, if you call a method, it appends another predicate to the Query, for example:

    /**
     * Only snapshots with a given commit property.
     * <br/><br/
     *
     * If this method is called multiple times,
     * <b>all</b> given properties must match with persisted commit properties.
     * @since 2.0
     */
    public QueryBuilder withCommitProperty(String name, String value) 

So multiple method calls are joined with logical AND.

@antch
Copy link
Author

antch commented Nov 6, 2019

If you call with a different property name that is true. But, if you call with the same property name multiple times, the last invocation wins. withCommithPropertyIn should work the same way.

@hungbang
Copy link

Hi @ALL
I have a similar question here: #919 . Thank you in advance

@lucas-prestes
Copy link

is anyone still working in this? If not I think I can contribute

@bartoszwalacik
Copy link
Member

Nobody is working on this, please go on

@bartoszwalacik
Copy link
Member

@lucas-prestes thanks for this contribution. I will release it tomorrow.

btw. your commits are not linked to your github account, it happens when you commit with an email not registered here https://github.com/settings/emails

If you link them, you will be automatically listed on https://javers.org as a contributor

@bartoszwalacik bartoszwalacik added fixed - waiting to be released and removed good first issue contribution wanted this feature is wanted but won't be implemented by core team due to limited resources labels Oct 17, 2021
@bartoszwalacik
Copy link
Member

usage:

        given:
        javers.commit('author', new SnapshotEntity(id: 1, intProperty: 2),[name:'Steve'])
        javers.commit('author', new SnapshotEntity(id: 1, intProperty: 3),[name:'John'])

        when:
        def snapshots = javers.findSnapshots(byInstanceId(1, SnapshotEntity)
                .withCommitPropertyIn('name', ['Steve','Bill'])
                .build())

        then:
        snapshots.size() == 1
        snapshots[0].getPropertyValue("intProperty") == 2

@bartoszwalacik
Copy link
Member

released in 6.4.1

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

No branches or pull requests

4 participants