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

Javaslang collections cannot be returned from paginating query methods [DATACMNS-1005] #1455

Closed
spring-projects-issues opened this issue Mar 6, 2017 · 13 comments
Assignees
Labels
in: repository in: web type: bug
Milestone

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Mar 6, 2017

Maciek Opała opened DATACMNS-1005 and commented

With the release of Ingalls, JavaSlang types (collections and Option) can be returned directly from spring-data @Repository - annotated components. Unfortunately, it turns out that if a in-@Repository-defined method has an argument of type @Pageable application fails at startup because of the following exception:

Caused by: java.lang.IllegalStateException: Method has to have one of the following return types! [interface org.springframework.data.domain.Slice, interface org.springframework.data.domain.Page, interface java.util.List]
	at org.springframework.data.repository.util.ClassUtils.assertReturnTypeAssignable(ClassUtils.java:120)
	at org.springframework.data.repository.query.QueryMethod.<init>(QueryMethod.java:81)

The following piece of code illustrates the problem:

interface EmailMessageRepository extends Repository<EmailMessage, Long> {

    List<EmailMessage> findBySentIsNull(Pageable pageable);
}

Affects: 1.13.1 (Ingalls SR1)

Referenced from: pull request #200

Backported to: 1.13.2 (Ingalls SR2)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 6, 2017

Oliver Drotbohm commented

Looks like our assertions for paginating query methods have to be updated

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 6, 2017

Maciek Opała commented

I'm keen on preparing a PR that will fix this issue, however would like to discuss it before. I guess it might not be enough to add JavaSlang types to the following piece of code (taken from QueryMethod):

if(!this.isStreamQuery()) {
   ClassUtils.assertReturnTypeAssignable(method, new Class[]{Slice.class, Page.class, List.class});
 }

?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 6, 2017

Daniel Dietrich commented

You may want to check if the returned collection is of type javaslang.collection.Seq. It is the most general interface for sequential collections like List and Vector

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 7, 2017

Oliver Drotbohm commented

Unfortunately it's not as simple as that'd introduce a strong dependency to Javaslang which we can't introduce as it currently is (and very probably will stay) an optional dependency. Nonetheless, should be fixable by introducing a dedicated method to return all supported types from QueryExecutionConverters

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 7, 2017

Maciek Opała commented

So, I will introduce private static final Set<Class<?>> ALLOWED_PAGEABLE_RETURN_TYPES; to QueryExecutionConverters which will by default have Slice.class, Page.class and List.class classes. Then, if javaslang is present, Seq.class will be added to the mentioned collection. This collection will be used in QueryMethod as an argument to ClassUtils invocation of assertReturnTypeAssignable. What do you think? Can I prepare a PR?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 7, 2017

Oliver Drotbohm commented

Sounds like a plan! :)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 7, 2017

Maciek Opała commented

I've started with creating a test (here) which I presume describes the expected behaviour well. The problem is that javaslang.collection.Seq is already supported in QueryExecutionConverters.UNWRAPPERS so in QueryMethod I will get unwrapped type at once. I'm not sure whether invocation of assertReturnTypeAssignable should be skipped at all (since returned result will be converted to Seq or some other logic should be done.

If assertReturnTypeAssignable is not skipped it fails in ClassUtils since passed type is - as I wrote - already unwrapped

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2017

Oliver Drotbohm commented

I guess we have to change QueryExecutionConverters quite a bit going forward to simplify the scenarios a bit as we're currently mixing up collection conversion with actual wrapping types (Optionals, Futures etc.). For now it should be sufficient to add a copy of assertReturnTypeAssignable that uses ….isSingleValue(…)
instead of ….supports(…). That's basically the same check but additionally returning whether we deal with a single-value wrapper. The original method doesn't seem to be used anywhere else but as it's public API, we might have someone out there relying on the old behavior.

You know what, I'd prefer that new method to live inside QueryMethod for now as it's very much tailored to that use case and can be package protected to not leak into the public API

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2017

Maciek Opała commented

I've implemented the solution using the clues you provided, please have a look here if this is what you expected. It it's ok I'll issue a PR at once

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2017

Oliver Drotbohm commented

We can't have a strong type reference to Seq anywhere outside QueryExecutionConverters. So that array of Slice, … Seq needs to come out of a static method of QEC.

Feel free to open the PR. It's easier to have implementation discussions right there

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2017

Maciek Opała commented

Here you go: #200

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 9, 2017

Oliver Drotbohm commented

Thanks, Maciek, I'll take it from here

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 9, 2017

Oliver Drotbohm commented

That's polished and merged. Thanks for your contribution, Maciek!

@spring-projects-issues spring-projects-issues added type: bug in: repository in: web labels Dec 30, 2020
@spring-projects-issues spring-projects-issues added this to the 2.0 M2 (Kay) milestone Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: repository in: web type: bug
Projects
None yet
Development

No branches or pull requests

2 participants