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

A new option (shrinkWhitespacesInSql) to remove extra whitespaces from SQL #1901

Merged
merged 12 commits into from May 18, 2020

Conversation

elfhazard
Copy link
Contributor

Hello mybatis!

I am a user of mybatis.
I used to post a question to mybatis google groups in the link below.

https://groups.google.com/forum/#!topic/mybatis-user/iVNejGgsAP4

And as an answer, I took a link to the 1126 issue and tested it.
#1126

However, the test code did not work.
So, after I forked the project myself, I analyzed it.

As a result, I found that all queries written in xml are handled by SqlSourceBuilder.
So, I will send you the source I modified.
I respectfully ask for a review.
Thank you.

@hazendaz
Copy link
Member

@harawata This LGTM. Build is fine (unrelated jdk 15 issue only). I think this is a pretty clean implementation and good reuse of code. I have had dba's complain about this as well. I can see the value here. I know you were involved with a lot of earlier comments. Do you think this is good to go?

@hazendaz
Copy link
Member

@elfhazard This looks good. I'm checking with @harawata before merging.

@elfhazard
Copy link
Contributor Author

@elfhazard This looks good. I'm checking with @harawata before merging.

@hazendaz Thank you for your review. Have a nice day!

Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

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

@hazendaz ,
Yeah, I can see the value. Let's proceed then.

@elfhazard ,
Thank you for the PR!
I have added some quick review comments.
Please let me know if you have questions, etc..

FYI, you can update this PR by...

  1. adding new commits to the branch in your local repo
  2. and pushing it to the GitHub branch elfhazard:feature/minifysql.

src/main/java/org/apache/ibatis/parsing/StringParser.java Outdated Show resolved Hide resolved
SqlSource sqlSource = sqlSourceBuilder.parse(sqlFromXml, null, null);
BoundSql boundSql = sqlSource.getBoundSql(null);
String actual = boundSql.getSql();
Assertions.assertNotEquals(sqlFromXml, actual);
Copy link
Member

Choose a reason for hiding this comment

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

Change this to assertEquals and specify the expected output explicitly.

@@ -113,6 +113,7 @@
protected boolean callSettersOnNulls;
protected boolean useActualParamName = true;
protected boolean returnInstanceForEmptyRow;
protected boolean minifySqlEnabled;
Copy link
Member

@harawata harawata May 17, 2020

Choose a reason for hiding this comment

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

'minifySql' implies a smart operation that does not change the functionality of the original SQL, but this could change the functionality if there are literals in the SQL (e.g. #459 ).
How about 'shrinkWhitespacesInSql'?

In any case, the suffix 'Enabled' is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I can see. The words you have presented are easier to understand than mine. I fix it quickly, thank you!!

@elfhazard elfhazard requested a review from harawata May 17, 2020 06:12
@elfhazard
Copy link
Contributor Author

Oh~! JDK 15 build test is removed!! Thank you! My local jdk 15 test also failed. I think error on maven plug in.

@elfhazard elfhazard requested a review from harawata May 17, 2020 09:42
@harawata
Copy link
Member

Thank you for the quick update, @elfhazard !

As this adds a new option, there are a few chores to do.

  • Add documentation (i.e. edit configuration.xml in src/site directory)
  • Update XmlConfigBuilderTest

Could you work on them if you still have some spare time?
It is totally fine if you are busy. I will do it myself later.

Regarding the documentation, only the English version is required, but please copy the English version to other languages' configuration.xml to help translators.

@elfhazard
Copy link
Contributor Author

@hazendaz I added doc with two languages that is en and ko.
Other languages is added as en.
And XmlConfigBuilderTest is update.
Have a good day!

@harawata
Copy link
Member

Thank you for the update, @elfhazard !

Regarding the doc, I have removed the term 'xml' from the explanation because this feature affects SQL in annotations as well.
Could you re-translate the Korean version when you have the time, please?

@hazendaz ,
I'll merge this tomorrow, probably.
Please let me know (or commit directly) if there is anything you noticed!

@hazendaz
Copy link
Member

@harawata Only thing that jumps out to me is the naming. Shrink whitespace vs removeExtraWhitespaces. I think those should be named the same to avoid confusing over the naming.

@elfhazard
Copy link
Contributor Author

@harawata I update korean description. Please check this commit. Thank you!

@harawata harawata self-assigned this May 18, 2020
@harawata harawata merged commit 63785f2 into mybatis:master May 18, 2020
This was referenced May 18, 2020
@harawata harawata changed the title option added that is remove white space from query string in xml files. A new option to remove extra whitespaces from SQL May 18, 2020
@harawata harawata changed the title A new option to remove extra whitespaces from SQL A new option (shrinkWhitespacesInSql) to remove extra whitespaces from SQL May 18, 2020
@harawata
Copy link
Member

Merged. Thank you for being super-responsive, @elfhazard !

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

Successfully merging this pull request may close these issues.

None yet

3 participants