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

Pageable warnings incosistent or wrong #3660

Closed
dsuepke-swisscom opened this issue Nov 6, 2024 · 11 comments
Closed

Pageable warnings incosistent or wrong #3660

dsuepke-swisscom opened this issue Nov 6, 2024 · 11 comments
Assignees
Labels
type: documentation A documentation update

Comments

@dsuepke-swisscom
Copy link

dsuepke-swisscom commented Nov 6, 2024

Hi,

I have two basically equivalent queries. One of them (findFoo) gives a warning on startup, one (findFoo2) doesn't. The warning seems to be correct, because in our project the query gets very slow.

Specific issues:

  • Either both should give a warning or none. They may be using different techniques in the background, but from a developer standpoint they do the same and should be treated the same.
  • I could not determine whether the warning for findFoo2is missing correctly or incorrectly.:
    • If it is working and thus no warning is given, then why does findFoo not work? Both of them are virtually the same.
    • If, however, the warning is rightfully missing, it needs to be shown

Minimal example:

Log output

 :: Spring Boot ::                (v3.3.5)

2024-11-06T08:53:52.872+01:00  WARN 63873 --- [demo] [           main] JpaBaseConfiguration$JpaWebConfiguration : spring.jpa.open-in-view is enabled by default. Therefore, database queries may be performed during view rendering. Explicitly configure spring.jpa.open-in-view to disable this warning
2024-11-06T08:53:53.107+01:00  WARN 63873 --- [demo] [           main] o.s.d.jpa.repository.query.NamedQuery    : Finder method public abstract org.springframework.data.domain.Page com.example.demo.FooRepository.findFoo(java.lang.String,org.springframework.data.domain.Pageable) is backed by a NamedQuery but contains a Pageable parameter; Sorting delivered via this Pageable will not be applied

FooRepository

@Repository
public interface FooRepository extends JpaRepository<FooEntity, Long>  {

	@Query(name = "FooEntity.findFoo", countName = "FooEntity.findFoo.count", nativeQuery = true)
	Page<FooEntity> findFoo(@Param("foo") String foo, Pageable pageable);

	@Query(value = "SELECT * FROM foo WHERE foo='foo'", countQuery = "SELECT count(*) FROM foo WHERE foo='foo'", nativeQuery = true)
	Page<FooEntity> findFoo2(@Param("foo") String foo, Pageable pageable);
}

FooEntity

@Entity
@Getter
@Setter
@NamedNativeQuery(name = "FooEntity.findFoo", query = "SELECT * FROM foo WHERE foo='foo'")
@NamedNativeQuery(name = "FooEntity.findFoo.count", query = "SELECT count(*) FROM foo WHERE foo='foo'")
public class FooEntity {

	@Id
	@Column
	@GeneratedValue(strategy = GenerationType.SEQUENCE)
	private long id;

	@Column
	private String foo;

}

build.gradle

plugins {
	id 'java'
	id 'org.springframework.boot' version '3.3.5'
	id 'io.spring.dependency-management' version '1.1.6'
}

group = 'com.example'
version = '0.0.1-SNAPSHOT'

java {
	toolchain {
		languageVersion = JavaLanguageVersion.of(21)
	}
}

configurations {
	compileOnly {
		extendsFrom annotationProcessor
	}
}

repositories {
	mavenCentral()
}

dependencies {
	implementation 'org.springframework.boot:spring-boot-starter-data-jpa'
	implementation 'org.springframework.boot:spring-boot-starter-web'
	compileOnly 'org.projectlombok:lombok'
	runtimeOnly 'com.h2database:h2'
	annotationProcessor 'org.projectlombok:lombok'
	testImplementation 'org.springframework.boot:spring-boot-starter-test'
	testRuntimeOnly 'org.junit.platform:junit-platform-launcher'
}

tasks.named('test') {
	useJUnitPlatform()
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 6, 2024
@Seol-JY
Copy link
Contributor

Seol-JY commented Nov 6, 2024

I’ve already completed the development to address this issue, and the changes are ready to be submitted in a PR. If the issue is valid, would it be okay for me to propose the changes via the PR?

@mp911de
Copy link
Member

mp911de commented Nov 7, 2024

@Seol-JY How do you plan to address the issue? What changes do you want to propose? Let's please discuss these upfront.

@dsuepke-swisscom to your question:

One query method uses named queries while the other declares a query string. That is a significant difference for us. To be able to apply sorting, we need to be in control over the actual query string. JPA's EntityManager allows us only calling EntityManager.createNamedQuery(queryName) after which we can only bind parameters and set limit/offset. We cannot alter the query itself.

In the second case, where you provide a SQL string, we are parsing the SQL query and injecting the given sort order.

Another aspect is that named queries are often associated times with additional information (e.g., @NamedNativeQuery(resultSetMapping = …, fetchSize = …,) so we cannot extract the query and run the query as it was a pure String-based query.

Let me know whether this makes sense.

@mp911de mp911de added the status: waiting-for-feedback We need additional information before we can continue label Nov 7, 2024
@Seol-JY
Copy link
Contributor

Seol-JY commented Nov 7, 2024

Thank you for your clarification, @mp911de

I had initially developed a solution assuming this was a valid issue, with changes ready for implementation. However, after reviewing your explanation of how named queries and string-based queries are handled differently - particularly regarding sort order capabilities and metadata handling - I now understand that the current warning behavior is intentional and technically correct.
I'll withdraw my proposed changes as they would have added unnecessary warnings that don't align with the actual technical implementation. Thank you for clarifying the architectural reasoning behind this difference.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 7, 2024
@dsuepke-swisscom
Copy link
Author

Hi, thanks to the both of you.
I understand the technical limitations and it makes sense, was kinda expecting this to be the underlying cause. So the warning itself is correct and I appreciate it. Possibly, there is still room for improvement:

  • It might be worthwhile to change the warning message in that case, as it states the issue but does not hint at a solution, which would be more helpful. It is still unclear for me what the recommended course of action is. Maybe it could add a link to documentation or suggest that this only works inline, or that sorting needs to be done manually?
  • In that regard, what actually is the recommended course of action? In the real application we are indeed using a resultSetMapping, inlining does not seem possible as the normal Query annotation does not support it.
  • We are following a clean-log policy, warnings and error should be fixed so we can quickly recognize actual issues. If the issues is solved, how would we get rid of the warning without suppressing all of them, as they are generally useful?

In summary, I think two follow-up issues arise from your helpful explanations: a) The warning should indicate at a solution rather than the problem, and b) there must be a way to get rid of the warning once it has been solved to keep a clean log.

@mp911de
Copy link
Member

mp911de commented Nov 8, 2024

In that regard, what actually is the recommended course of action? In the real application we are indeed using a resultSetMapping, inlining does not seem possible as the normal Query annotation does not support it.

Thanks for the detail. Have you tried switching to our new @NativeQuery annotation (available in the latest milestones and RC1 release). It allows to specify @NativeQuery(sqlResultSetMapping=…).

Alternatively, you could switch to Limit and ScrollPosition parameters while returning Window<T>.

Once you can confirm that @NativeQuery(sqlResultSetMapping=…) is working for you, we could update our log message to include this suggestion.

@mp911de mp911de added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Nov 8, 2024
@dsuepke-swisscom
Copy link
Author

dsuepke-swisscom commented Nov 15, 2024

Sorry, had to work on another project for a few days. I would like to try @NativeQuery, but I cannot figure out how to import that into our gradle project. We have these two, which seem relevant:

plugins {
	id 'org.springframework.boot' version '3.3.0'
	...
}
dependencies {
	implementation 'org.springframework.boot:spring-boot-starter-data-jpa'
	...
}

I cannot find an RC on mavencentral. I found some release notes on the JPA site, but no instructions on how to use it. Could you kindly help me on how to use the mentioned RC for NativeQuery?

EDIT: Changed the plugin, copied the wrong line.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 15, 2024
@mp911de
Copy link
Member

mp911de commented Nov 15, 2024 via email

@mp911de mp911de self-assigned this Nov 18, 2024
@mp911de mp911de added type: documentation A documentation update and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Nov 18, 2024
@dsuepke-swisscom
Copy link
Author

I am having trouble getting the RC to work. Gradlew gives me error messages ("Problems reading data from Binary store") and there is no obvious solution. I might have to wait till the RC is released an we have a ticket to upgrade.

@mp911de
Copy link
Member

mp911de commented Dec 2, 2024

3.4 is GA now.

@dsuepke-swisscom
Copy link
Author

Thanks, the upgrade worked now. Also, using @NativQuery and inlining the queries got rid of the startup warning, but now the queries fail with Unknown column 't.bank' in 'order clause', even when I have no order by. So I'm not sure of the solution, as the query works fine when using @Query(nativeQuery = true).

@mp911de mp911de added this to the 3.4.1 (2024.1.1) milestone Dec 9, 2024
@mp911de
Copy link
Member

mp911de commented Dec 9, 2024

but now the queries fail with Unknown column 't.bank' in 'order clause', even when I have no order by. So I'm not sure of the solution, as the query works fine when using @Query(nativeQuery = true).

If that issue persists, feel free to file a bug report along with a minimal reproducer.

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

No branches or pull requests

4 participants