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

GH-1006 included QueryMappingConfiguration for all part tree queries #1893

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Sep 20, 2024

Solving #1006 issue

To ease the review - actually, we already had the QueryMappingConfiguration applied for PartTree queries, but only for those that are declared in the repository and execute via PartTreeJdbcQuery#execute. For the predefined methods, that resolve into SimpleJdbcRepository methods we did not have QueryMappingConfiguration applied. So I've just covered this gap and added some tests to verify the expected behavior.

PS: For StringBasedJdbcQuery no changes required.

@mipo256
Copy link
Contributor Author

mipo256 commented Oct 1, 2024

Resolved the conflict after Iterable --> List return type change for DefaultDataAccessStrategy.
Kind reminder for the review @schauder :)

@schauder
Copy link
Contributor

schauder commented Oct 2, 2024

This PR fails DependencyTests.acrossModules. Could you please fix that?

@schauder schauder added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 2, 2024
@mipo256
Copy link
Contributor Author

mipo256 commented Oct 2, 2024

@schauder
It actually is a bit strange that we have the failure in archunit test for cycles between modules, since only spring-data-jdbc module was affected. I'll give it a look and report below.

@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 Oct 2, 2024
@schauder
Copy link
Contributor

schauder commented Oct 2, 2024

There are components (e.g. repository, mapping that exist in various modules. You get circles among these when you have a dependency in one direction in one module (possibly relational or commons) but in the opposite direction in another module (e.g. jdbc)

@mipo256
Copy link
Contributor Author

mipo256 commented Oct 19, 2024

@schauder so, the dependency error was fixed. The solution was to move QueryMappingConfiguration from:

package org.springframework.data.jdbc.repository;

to

package org.springframework.data.jdbc.core.convert;

It seems to make sense, since the QueryMappingConfiguration is really a part of the core conversion mechanism, not really the repository package.

What are your thoughts on this? @schauder

@mp911de mp911de added the has: breaking-change An issue that is associated with a breaking change. label Oct 21, 2024
@mp911de
Copy link
Member

mp911de commented Oct 21, 2024

I updated our labels as this change is a breaking one by moving public types around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has: breaking-change An issue that is associated with a breaking change. status: feedback-provided Feedback has been provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants