Skip to content

Pageable not supported with String @Query #276

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

Closed
marchermans opened this issue Jan 15, 2020 · 11 comments
Closed

Pageable not supported with String @Query #276

marchermans opened this issue Jan 15, 2020 · 11 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@marchermans
Copy link

Description:

Creating a repository method as follows:

    @Override
    @Query("Select * from game_version g")
    Flux<GameVersionDMO> findAll(Pageable pageable);

Should generally append paging information to the query that is send to the database, causing paging information to be resolved on the DB instead of in the JVM for a much better performance. This is also specified in the R2DBC Docu when it comes to JDBC compatibility and efforts have been made to support it: EG: R2DbcQueryMethod class which checks for valid return types etc.

However right now this query just plainly crashes the JVM cause R2DBC attempts to bind the Pageable in this query while there is no bindable parameter in the query itself.
This is caused by "StringBasedR2DBCQuery" does not check if a given Type that is passed in as parameter is a "Special"-None bindable parameter.

Required fix:

  1. Make R2DBC inject special parameter information
  2. Make "StringBasedR2dbcQuery" care for the fact that it is attempting to inject "Special"-Parameters and skip them
@mp911de
Copy link
Member

mp911de commented Jan 15, 2020

We have no general possibility to append pagination information to the query when using @Query and Pageable. The reason is that SQL to use pagination varies (SELECT TOP …, LIMIT … OFFSET, LIMIT <n>, <m>) across databases and we have no generic rewrite mechanism nor we want to implement one as the complexity would explode. Please rather use either SpEL or bind parameters to render the correct SQL in your @Query annotations.

Using Pageable with JPA is possible as JPA abstracts its query into an object where limit/offset parameters can be provided.

@mp911de mp911de added the for: team-attention An issue we need to discuss as a team to make progress label Jan 15, 2020
@marchermans
Copy link
Author

marchermans commented Jan 16, 2020

Okey I can understand your position in the sense that this project is still experimental.
Let me then however make some comments on the situation:

  1. You are not supporting the use of Pageable or Sort in any shape or form, that is fine. However I can not even bind them manually since they are excluded from being bindable in the first place by Spring Data, causing the StringBased Query still to crash because it does not exclude them during its binding attempt causing an index out of range exception. This is in my opinion regardless of the fact that you do not support them a Bug, and should be fixed.
  2. Your Reference documentation should then not mention JPA Only features at all: https://docs.spring.io/spring-data/r2dbc/docs/1.0.x/reference/html/#repositories in particular as a whole needs to go, it misleading and confusing at best.
  3. You suggest using SPeL, this would be somewhat of a compromise, however based on the issue @Query definitions with SpEL expressions #164 it seems like that the required functionality from SPeL that is needed to implement this using it is not supported by Spring-Data-R2DBC. Making that option not really worthwhile.
  4. Your suggestion to use SPeL or manually binding (if either one of these options would be available, which they aren't see my points 1) and 3)) is not feasible, Pageable and Sort are extremely complicated logical operations to included them in a query: EG How do you support both not paged and paged querries in one go, without making a mess of the query in the first place.

This said, it sounds like you are never expecting this to be part of R2DBC, making this project utterly useless in situations where it would be very useful, communicating with large databases: How do you expect me to handle this in situations where i have 100000+ records in a table, from which I potentially need all of them or just parts of them (AKA internal usage to handle exporting etc, and exposing them via a rest API). This alone would make the complexity of the Repository explode into a dimension which not comprehensible anymore. Cause I can not use the Fluxes internal capability to limit and skip entries, since this would still load the entries into memory then discarding them, which is simply not an option.

That said, from the way this project is represented it presents itself as an alternative to JPA in the future (which means it WILL need native support for these scenarios eventually). If this is not the case this should be extremely well communicated: AKA this project is not meant to replace JPA and JPAs high performance characteristics when it comes to communication with the database.

That said, i fully support his project it is an amazing fresh new breeze in the landscape of datacommunication with databases which is extremely welcome.

Greets Orion,

PS.: This does not mean that you can close this issue. The Bug With StringBasedR2DBCQuery not allowing me to bind them in the first place still exists, and I think that Special parameters are needed to give this project any chance.

@IvanKonevJr
Copy link

Dear Orion! Everything is supported, the technology is really great - I use it with pleasure in all projects. You can use pages and any sorts in the R2dbcDslRepository, as well as any DSL construct from the web or from code using this extension: https://github.com/SevenParadigms/r2dbc-dsl

@marchermans
Copy link
Author

marchermans commented Jan 16, 2020

@LaoTsing That is not really what i am looking for sadly.......
I am not looking for something that generates my core SQL code for me. I just want it to take the special parameters into account so this project becomes somewhat viable and useful.

Do not get me wrong, there is a lot of stuff that you can already do with this, and that makes it extremely useful, however right now it is, at least in my opinion, at a point where it is working well enough to be called spring experimental, but definitely not well enough, and is not flushed out enough, to be actually part of the spring family. And the team behind this project just needs to make it more clear where they are going -> Do they want to become a valid way to access dba data from a JVM in somewhat of a familiar way like JPA, or do they want to be a pure datamapper, that maps the results of a query to something else. Cause right now they are neither, and that is the point. They will have to make a choice and clearly communicate this with the community.

@IvanKonevJr
Copy link

@OrionDevelopment Rather, the R2DBC is positioning itself for highly loaded systems and therefore it will never become an ORM, it may go some part of the MyBatis path, but in the end it will become functional enough to replace Hibernate...

And which ones are needed of the special parameters for you?

@marchermans
Copy link
Author

Pageable and Sort, i understand that it might never become an ORM, but right now the documentation of Spring-R2DBC at least highly suggest it is one, additionally it crashes right now, which is simply not acceptable.

@marchermans
Copy link
Author

Mainly pageable.....

@IvanKonevJr
Copy link

and what does this syntax not suit? :)

repository.findAll(R2dbcDsl.create().id(100L), serverRequest.getPageable())

@mp911de mp911de changed the title [Bug]: Failure in handling special parameters. Pageable not supported with String @Query Apr 21, 2020
@mp911de mp911de added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we need to discuss as a team to make progress labels Apr 21, 2020
@mp911de
Copy link
Member

mp911de commented Apr 21, 2020

After discussing this issue with our team we decided to not support Pageable with string-based query methods. We would be required to apply parsing or other types of SQL transformation and Spring Data is not a SQL transformation library. Rather, we aim for making the most common use-cases simple. With the introduction of query derivation (#282), we accept Pageable as argument as the SQL query is constructed behind the scenes.

For ever other use-case, please use custom implementation methods.

@mp911de mp911de closed this as completed Apr 21, 2020
@arthurgrig
Copy link

Is there a way to pass in sorting parameters? passing other pagination params offset and limit into my query works just fine. order by param is ignored.

trying something like:

order by :orderBy

@IvanKonevJr
Copy link

IvanKonevJr commented Jan 28, 2022

Is there a way to pass in sorting parameters? passing other pagination params offset and limit into my query works just fine. order by param is ignored.

trying something like:

order by :orderBy

Arthur, in @query method no way, but you can use:

repository.findAll(Dsl.create().equals(manual, 100).sorting("name","desc").sorting("desc","asc"))

from extension https://github.com/SevenParadigms/r2dbc-dsl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants