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

FMWK-438 Add tests and javadocs for comparing Collections #740

Merged
merged 21 commits into from
May 15, 2024

Conversation

agrgr
Copy link

@agrgr agrgr commented May 12, 2024

Update repository queries validation to only support Lists with comparison operators instead of Collection

@agrgr agrgr requested review from roimenashe and reugn May 12, 2024 08:40
@agrgr agrgr marked this pull request as ready for review May 12, 2024 09:11
Comment on lines 95 to 97
if (queryParameters.stream().anyMatch(param -> !(param instanceof List<?>))) {
throw new IllegalArgumentException(queryPartDescription + ": only Lists can be compared");
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider extracting to a util method and reusing it.

private static boolean propertyTypeAndFirstParamAssignableToNumber(Class<?> propertyType,
List<Object> queryParameters) {
return queryParameters != null
&& queryParameters.size() > 0
Copy link
Member

Choose a reason for hiding this comment

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

Use !queryParameters.isEmpty()

@agrgr agrgr changed the title FMWK-438 Validate that only List instances can be accepted with comparison operators FMWK-438 Validate that only List instances can be accepted with comparison operators, not Collection May 12, 2024
@roimenashe
Copy link
Member

@agrgr As discussed, removing the support for collections comparison other than list (e.g. sets) is problematic, it introduces a breaking change and it also removes a useful feature. We will talk about it tomorrow (and cover additional sets comparison scenarios such as GT, LE etc...)

@agrgr
Copy link
Author

agrgr commented May 15, 2024

@agrgr As discussed, removing the support for collections comparison other than list (e.g. sets) is problematic, it introduces a breaking change and it also removes a useful feature. We will talk about it tomorrow (and cover additional sets comparison scenarios such as GT, LE etc...)

Changing the PR as discussed.

@agrgr agrgr changed the title FMWK-438 Validate that only List instances can be accepted with comparison operators, not Collection FMWK-438 Add tests and javadocs for comparing Collections May 15, 2024
PriorityQueue<Integer> queueToCompareWith = new PriorityQueue<>(Set.of(3, 1, 2, 4, 0));
List<Person> persons4 = repository.findByIntSetGreaterThan(queueToCompareWith);
assertThat(persons4).contains(dave);

Copy link
Member

Choose a reason for hiding this comment

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

Redundant new line

PriorityQueue<Integer> queueToCompareWith = new PriorityQueue<>(Set.of(3, 1, 2, 4, 0));
List<Person> persons4 = repository.findByIntSetGreaterThanEqual(queueToCompareWith);
assertThat(persons4).contains(dave);

Copy link
Member

Choose a reason for hiding this comment

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

Redundant new line

@agrgr agrgr merged commit 7c97e7a into main May 15, 2024
7 checks passed
@agrgr agrgr deleted the FMWK-438-compare-lists branch May 15, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants