A new option (shrinkWhitespacesInSql) to remove extra whitespaces from SQL #1901
Conversation
…m query string in xml file.
|
@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? |
|
@elfhazard This looks good. I'm checking with @harawata before merging. |
@hazendaz Thank you for your review. Have a nice day! |
There was a problem hiding this comment.
@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...
- adding new commits to the branch in your local repo
- and pushing it to the GitHub branch
elfhazard:feature/minifysql.
src/test/java/org/apache/ibatis/builder/SqlSourceBuilderTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/ibatis/builder/SqlSourceBuilderTest.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); |
There was a problem hiding this comment.
Change this to assertEquals and specify the expected output explicitly.
src/test/java/org/apache/ibatis/builder/SqlSourceBuilderTest.java
Outdated
Show resolved
Hide resolved
| @@ -113,6 +113,7 @@ | |||
| protected boolean callSettersOnNulls; | |||
| protected boolean useActualParamName = true; | |||
| protected boolean returnInstanceForEmptyRow; | |||
| protected boolean minifySqlEnabled; | |||
There was a problem hiding this comment.
'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.
There was a problem hiding this comment.
Oh, I can see. The words you have presented are easier to understand than mine. I fix it quickly, thank you!!
src/test/java/org/apache/ibatis/builder/SqlSourceBuilderTest.java
Outdated
Show resolved
Hide resolved
|
Oh~! JDK 15 build test is removed!! Thank you! My local jdk 15 test also failed. I think error on maven plug in. |
|
Thank you for the quick update, @elfhazard ! As this adds a new option, there are a few chores to do.
Could you work on them if you still have some spare time? Regarding the documentation, only the English version is required, but please copy the English version to other languages' |
|
@hazendaz I added doc with two languages that is en and ko. |
|
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. @hazendaz , |
|
@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. |
|
@harawata I update korean description. Please check this commit. Thank you! |
harawata
changed the title
harawata
changed the title
|
Merged. Thank you for being super-responsive, @elfhazard ! |
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.