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

Expose query method metadata #3259

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Mar 21, 2025

I think it is worth to make the accessors public (or protected maybe) for method and RepositoryMetadata. The Spring Data JDBC for instance, because it cannot access the method field in the QueryMethod has to re-declare it inside the JdbcQueryMethod, so it is clearly redundant, since the method already is present in the parent.

P.S: This is also would help us implemented the YdbQueryMethod in the Spring Data JDBC dialect for YDB database.

CC: @mp911de

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 21, 2025
@mp911de
Copy link
Member

mp911de commented Mar 24, 2025

QueryMethod has to re-declare it inside the JdbcQueryMethod,

This is by design. We do not want to expose constructor arguments as public API. If you want to expose these, please define appropriate methods in your implementation class.

However, JdbcQueryLookupStrategy (creating JdbcQueryMethod) ships with a multitude of context (8 constructor args) and it is package-private. How do you want to create your own YdbQueryMethod? I don't really see how that would work.

Going forward, we do not provide SQL types or type names for MapSqlParameterSource when binding parameters (specifically expressions or scalar values). The only thing that comes to my mind is using JdbcValue. You could add a YdbLimit class to your dialect (YdbLimit.of(…)).

@mp911de mp911de added status: on-hold We cannot start working on this issue yet status: waiting-for-feedback We need additional information before we can continue labels Mar 24, 2025
@mipo256
Copy link
Contributor Author

mipo256 commented Mar 24, 2025

This is by design. We do not want to expose constructor arguments as public API. If you want to expose these, please define appropriate methods in your implementation class.

Okay, I get it that this is an arguable idea to expose the method and repositoryMetadata via public getters, I kind of agree.

But let's at least make the getters protected. The child of course aware about the method and repositoryMetadata, since it provided them to the parent via super() call, but in this case the child has to re-declare the method and repositoryMetadata inside of it.

So we basically needlessly waste space for two more references, without getting any extra benefits. And this complicates the implementation of the child, btw.

@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 Mar 24, 2025
@mipo256
Copy link
Contributor Author

mipo256 commented Mar 24, 2025

However, JdbcQueryLookupStrategy (creating JdbcQueryMethod) ships with a multitude of context (8 constructor args) and it is package-private. How do you want to create your own YdbQueryMethod? I don't really see how that would work.

Well, we do not use the JdbcQueryLookupStrategy directly, we extend it, see here the corresponding class in the PR .

And, again, we of course can re-declare the references to fields that we pass to parent via super() - but why? I do not really see any compelling reason to do so to be honest. That is why the following change was proposed. I think it is fine to give that to children, they need it.

@mipo256
Copy link
Contributor Author

mipo256 commented Mar 24, 2025

Going forward, we do not provide SQL types or type names for MapSqlParameterSource when binding parameters (specifically expressions or scalar values).

I am aware of that. That is why we have overridden the createParamaters.

The only thing that comes to my mind is using JdbcValue. You could add a YdbLimit class to your dialect (YdbLimit.of(…)).

Unfortunately, that would not help. Changes in dialect are static. The idea is that the user must be able to dynamically provide the SQLType of the underlying parameter.

@mp911de
Copy link
Member

mp911de commented Mar 24, 2025

But let's at least make the getters protected. The child of course aware about the method and repositoryMetadata, since it provided them to the parent via super() call, but in this case the child has to re-declare the method and repositoryMetadata inside of it.

If a subclass wants to expose these, then it should introduce dedicated methods.

And, again, we of course can re-declare the references to fields that we pass to parent via super() - but why? I do not really see any compelling reason to do so to be honest.

Encapsulation and design. Not everything a constructor receives is intended to be exposed publicly. We hide certain aspects to avoid coupling (everything that is even remotely visible is going to be used. Everything being used and then changed generates tickets that ask for a reply).

Well, we do not use the JdbcQueryLookupStrategy directly, we extend it, see here the corresponding class in the PR .

🙈 JdbcQueryLookupStrategy is package-protected by design and to give us flexibility to change arguments without guaranteeing that it is a stable, public API.

The idea is that the user must be able to dynamically provide the SQLType of the underlying parameter.

It sounds that this is the actual requirement. I wondered already whether we could come up with a mechanism to describe certain parameter aspects (e.g. in Cassandra we have @CassandraType that can be used also on repository query method parameters). I could well imagine a proper @SqlType("uint64") approach and some Dialect-specific resolver that takes a type string and resolves it into java.sql.SQLType.

That would be IMO a much cleaner approach requiring less reliance on internals.

@mipo256
Copy link
Contributor Author

mipo256 commented Mar 24, 2025

Encapsulation and design. Not everything a constructor receives is intended to be exposed publicly.

I'm not asking to make the accessors public, let's make them protected.

We hide certain aspects to avoid coupling (everything that is even remotely visible is going to be used. Everything being used and then changed generates tickets that ask for a reply).

I perfectly know that. The problem is that there is a balance. You can hide everything out, ad make the framework difficult to extend, and that would lead to fewer users. Or you can expose a lot, and then the framework is highly customizable and etc. but changing any api results in a breaking change, and we go down that path. I understand it. My point is - there is a balance, and I honestly do not really think that making accessors protected is that much of burden in terms of backward compatibility.

It sounds that this is the actual requirement. I wondered already whether we could come up with a mechanism to describe certain parameter aspects (e.g. in Cassandra we have @CassandraType that can be used also on repository query method parameters). I could well imagine a proper @SqlType("uint64") approach and some Dialect-specific resolver that takes a type string and resolves it into java.sql.SQLType.

The spring-data-cassandra does exactly what we're doing, exactly that. We can, of course, add a new annotation in the spring-data-jdbc module like @JdbcType and then consider it when resolving the SQLType.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided status: on-hold We cannot start working on this issue yet status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants