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

Custom extension of Pageable as parameter causes query method to be rejected [DATACMNS-1383] #1819

Closed
spring-projects-issues opened this issue Aug 30, 2018 · 8 comments
Assignees
Labels
in: repository type: bug

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Rimal opened DATACMNS-1383 and commented

If a child class of Pageable is passed in a page query, then there is an error in server startup

Caused by: java.lang.IllegalArgumentException: Paging query needs to have a Pageable parameter! Offending method public abstract com.app.backend.repository.pagination.AppPage com.app.backend.repository.BaseRepository.findAll(com.app.backend.repository.pagination.AppPageRequest)
    at org.springframework.util.Assert.isTrue(Assert.java:116) ~[spring-core-5.0.8.RELEASE.jar:5.0.8.RELEASE]
    at org.springframework.data.repository.query.QueryMethod.<init>(QueryMethod.java:99) ~[spring-data-commons-2.0.9.RELEASE.jar:2.0.9.RELEASE]
    at org.springframework.data.neo4j.repository.query.GraphQueryMethod.<init>(GraphQueryMethod.java:41) ~[spring-data-neo4j-5.0.9.RELEASE.jar:5.0.9.RELEASE]
    at org.springframework.data.neo4j.repository.query.GraphQueryLookupStrategy.resolveQuery(GraphQueryLookupStrategy.java:49) ~[spring-data-neo4j-5.0.9.RELEASE.jar:5.0.9.RELEASE]

 

Here is a sample code:

public class AppPageRequest extends PageRequest implements Pageable {

  private AppPageRequest(int page, int size, Sort sort) {
    super(page - 1, size, sort);
  }

  public static AppPageRequest of(int page, int size) {
    return of(page, size, Sort.unsorted());
  }

  public static AppPageRequest of(int page, int size, Sort sort) {
    return new AppPageRequest(page, size, sort);
  }
}

@NoRepositoryBean
public interface BaseRepository<T, ID extends Serializable> extends Neo4jRepository<T, ID> {

  Page<T> findAll(AppPageRequest appPageRequest);
}

@NoRepositoryBean
public class BaseRepositoryImpl<T, ID extends Serializable> extends SimpleNeo4jRepository<T, ID> implements BaseRepository<T, ID> {

  public BaseRepositoryImpl(Class<T> domainClass, Session session) {
    super(domainClass, session);
  }

  public Page<T> findAll(AppPageRequest appPageRequest) {
    return super.findAll(appPageRequest);
  }
}

Affects: 2.0.9 (Kay SR9)

Backported to: 2.0.10 (Kay SR10), 1.13.15 (Ingalls SR15)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Oliver Drotbohm commented

It looks like BaseRepository isn't even properly configured to be used by the infrastructure as otherwise, there wouldn't be a QueryMethod created in the first place. Would you mind checking this?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Rimal commented

I have the BaseRepositoryImpl set as the repositoryBaseClass. Here is the annotation added in the project config

@EnableNeo4jRepositories(basePackages = "com.app.backend.repository", repositoryBaseClass = BaseRepositoryImpl.class)

 

Is there anything else, I should check?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Oliver Drotbohm commented

What's the actual repository interface that this is bootstrapped for. BaseRepository apparently is supposed to be an intermediate, right? Any chance you provide a sample project that we can play with?

I have a fix for the too strict parameter check but that method should be considered a query method in the first place

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Rimal commented

This was a project on spring SDN. Neo4jRepository is a repository provided in it directly. The things were working if I passed Pageable directly. Only for implementations of Pageable, it is throwing this error on server startup.

The issue got fixed when I added support in Parameters for implementations of Pageable. Have also submitted a pull request for the same. 

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Oliver Drotbohm commented

As indicated, I already have a fix for the detection problem, thanks. I'd just like to understand why your custom ….findAll(…) method is considered a query method when it clearly shouldn't be. Thus a sample project to reproduce that scenario would be helpful

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Oliver Drotbohm commented

You should see this working on the latest snapshots. If you'd like me to look into the bootstrap issue, feel free to add a sample project into it

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Rimal commented

While writing a sample project, I realised, you were right. It was working fine!

My original project is working with the custom fix I have submitted in the pull request. Going back in git history, found that when I was facing the issue, repositoryBaseClass = BaseRepositoryImpl.class config was not there.

I guess, we added it later as a part of some other fix.

Apologies!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2018

Oliver Drotbohm commented

No worries, Rimal. Happy that we've got it working for you. The fix you suggested is still very valuable as we should support this scenario for actual query methods, too. I.e. while the scenario you documented here should not cause a problem if configured properly, it will still come up if someone tries to use a custom extension of Pageable in a real query method.

Happy coding!

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

No branches or pull requests

2 participants