Skip to content

Favor SqlIdentifier.getReference() instead of getReference(IdentifierProcessing) #1458

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Favor SqlIdentifier.getReference() instead of getReference(IdentifierProcessing) #1458

wants to merge 5 commits into from

Conversation

kurtn718
Copy link
Contributor

Change to deprecate redundant getReference(IdentifierProcessing) method.

I split out the commits to make this change easier to review.

The first commit after the branch preparation outlines the changes to actual implementation. Second commit is Unit Tests supporting change, and third commit is change/eliminate all usages of the deprecated API.

I fixed the implementation of the deprecated method to actually use the IdentifierProcessing parameter. If we want to leave the implementation unchanged, please let me know.

Will squash commits after making any other needed changes.

kurtn718 added 4 commits March 20, 2023 16:51
@kurtn718
1110 - Prepare branch
aabca9d
@kurtn718
Deprecate redundant getReference(IdentifierProcessing) method
3ca3d71 
The method also fixed the bug of this method not honoring the IdentifierProcessing parameter, so the functionality is now identical to the toSql(IdentifierProcessing) method.

Closes #1110
@kurtn718
Unit Test changes
e1c76b7
@kurtn718
Changes to not use deprecated method. To avoid breaking existing usag…
a92b0e4 
…es of getReference(identifierProcessing) as it did not do identifier processing, we call the getReference() method which does not do any.
@kurtn718 kurtn718 requested a review from schauder March 21, 2023 01:23
@kurtn718 kurtn718 mentioned this pull request Mar 21, 2023
Closed
@kurtn718 kurtn718 changed the title Issue/1110 Issue #1110 - SqlIdentifier has redundant getReference(IdentifierProcessing) method #1457 Mar 21, 2023
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 21, 2023
mp911de
mp911de reviewed Mar 21, 2023
View reviewed changes
...nal/src/main/java/org/springframework/data/relational/core/mapping/DerivedSqlIdentifier.java Outdated
@@ -65,8 +65,9 @@ public String toSql(IdentifierProcessing processing) {
}

@Override
@Deprecated(since="3.0.5", forRemoval = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should deprecate that method with the next feature release, not with a bugfix. That would be since="3.1"

Copy link
Contributor

@schauder schauder Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it should be forRemoval = true since we want to drop it eventually. This applies to all deprecations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change. Wasn't sure which version we wanted - and we had just done 3.0.4

mp911de
mp911de reviewed Mar 21, 2023
View reviewed changes
...ational/src/main/java/org/springframework/data/relational/core/sql/DefaultSqlIdentifier.java Outdated
}

@Override
@Deprecated(since="3.0.5", forRemoval = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See deprecation notice above.

mp911de
mp911de reviewed Mar 21, 2023
View reviewed changes
...ata-relational/src/main/java/org/springframework/data/relational/core/sql/SqlIdentifier.java Outdated
@@ -77,9 +78,12 @@ public String toString() {
* Return the reference name after applying {@link IdentifierProcessing} rules. The reference name is used for
* programmatic access to the object identified by this {@link SqlIdentifier}.
*
* @deprecated Use the toSql(IdentifierProcessing processing) method instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind the formatting, @deprecated comes after @return, we should also include that the deprecation will be since 3.1 in place (@deprecated since 3.1, use the #getReference() method instead.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the replacement is getReference() not toSql(IdentifierProcessing)

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 21, 2023
@mp911de mp911de linked an issue Mar 21, 2023 that may be closed by this pull request
SqlIdentifier has redundant getReference(IdentifierProcessing) method #1110
Closed
@mp911de mp911de changed the title Issue #1110 - SqlIdentifier has redundant getReference(IdentifierProcessing) method #1457 Favor SqlIdentifier.getReference() instead of getReference(IdentifierProcessing) Mar 21, 2023
schauder
schauder requested changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some more comments and also chatted with Mark, so we all pull in the same direction.

Please add your changes addressing the comments as an additional commit and ping me once your done.

...ata-relational/src/main/java/org/springframework/data/relational/core/sql/SqlIdentifier.java
@@ -90,7 +94,7 @@ public String toString() {
* @see IdentifierProcessing#NONE
*/
default String getReference() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give this a proper JavaDoc comment.

After talking to Mark I now again understand what the getReference vs toSql is about.

toSql is to be used when rendering an identifier in SQL, as in select id from someTable

getReference is used when accessing a column in a ResultSet here we must not apply any quoting.

This should be reflected in the JavaDoc comments of the two methods.

...ata-relational/src/main/java/org/springframework/data/relational/core/sql/SqlIdentifier.java Outdated
@@ -77,9 +78,12 @@ public String toString() {
* Return the reference name after applying {@link IdentifierProcessing} rules. The reference name is used for
* programmatic access to the object identified by this {@link SqlIdentifier}.
*
* @deprecated Use the toSql(IdentifierProcessing processing) method instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the replacement is getReference() not toSql(IdentifierProcessing)

schauder pushed a commit that referenced this pull request Mar 23, 2023
@kurtn718 @schauder
Deprecate redundant getReference(IdentifierProcessing) method.
18eda61 
Closes #1110
Original pull request #1458
schauder added a commit that referenced this pull request Mar 23, 2023
@schauder
Polishing.
58a67ce 
Added `@author` tags.
Undo the change in behaviour of getReference(IdentifierProcessing).

See #1110
Original pull request #1458
@schauder
Copy link
Contributor

This is polished and merged.
Thanks.

I know I wrote in some textbox, that we shouldn't change the behaviour of getReference(IdentifierProcessing) but it's clearly not here 😕. I wonder where I put that comment 🤔.

Anyway, I undid that change and also added you in `@author tags.

@schauder schauder closed this Mar 23, 2023
@schauder schauder deleted the issue/1110 branch March 23, 2023 09:56
@mp911de mp911de added this to the 3.1 RC1 (2023.0.0) milestone Mar 23, 2023
@schauder schauder mentioned this pull request Mar 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de mp911de left review comments

@schauder schauder schauder requested changes

Assignees
No one assigned
Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.1 RC1 (2023.0.0)
Development

Successfully merging this pull request may close these issues.

SqlIdentifier has redundant getReference(IdentifierProcessing) method
4 participants
@kurtn718 @schauder @mp911de @spring-projects-issues