Reuse SqlNode instance created for virtually empty text node

[nieqiurong]

In the memory analysis, it was found that there were some blank nodes in mybatis,
Some are line breaks followed by blank strings, which are considered as blank nodes. However, the space occupied by these nodes is meaningless, and it is possible to cache these blank nodes during parsing and only retain one instance

example:

<script>
select * from user
<where>
<if test="1==1">and id = 1</if>
<if test="1==1">and id > 0</if>
</where>
</script>

For example, when parsing the sample script, four line break tags will be created. This is just a simple example, and we will use more dynamic tags in the project.

This modification will not change the original script content.

[coveralls]

coverage: 87.245% (+0.01%) from 87.235%
when pulling f28e63a on nieqiurong:202502222213
into 1a145cd on mybatis:master.

[harawata]

Thank you for the PR, @nieqiurong !

I really liked the effect of your original PR #3349 which significantly reduces the number of SqlNodes to process.
And I wasn't sure why using <where> broke #3349, so I dug a little.

After evaluating each SqlNode, MyBatis concatenates the result strings.
In MixedSqlNode, it is done by DynamicContext#appendSql() which internally uses StringJoiner#add(), so it adds an extra space when concatenating SQL strings.
However, in TrimSqlNode (which is WhereSqlNode's parent class), FilteredDynamicContext#appendSql() is called and the result strings are concatenated without the extra space.
This is why #3349 broke when <where>, <set> or <trim> is used.

I think, for consistency, TrimSqlNode should add the extra space like MixedSqlNode does. Then we can (re)apply #3349 .
I still haven't made my mind about what to do, but could you take a look at #3422 and see if there is anything I am missing?

Cc: @epochcoder

[nieqiurong]

Thank you for the PR, @nieqiurong !

I really liked the effect of your original PR #3349 which significantly reduces the number of SqlNodes to process. And I wasn't sure why using <where> broke #3349, so I dug a little.

After evaluating each SqlNode, MyBatis concatenates the result strings. In MixedSqlNode, it is done by DynamicContext#appendSql() which internally uses StringJoiner#add(), so it adds an extra space when concatenating SQL strings. However, in TrimSqlNode (which is WhereSqlNode's parent class), FilteredDynamicContext#appendSql() is called and the result strings are concatenated without the extra space. This is why #3349 broke when <where>, <set> or <trim> is used.

I think, for consistency, TrimSqlNode should add the extra space like MixedSqlNode does. Then we can (re)apply #3349 . I still haven't made my mind about what to do, but could you take a look at #3422 and see if there is anything I am missing?

Cc: @epochcoder

If empty rows are removed, it can lead to some extreme cases.

  @Test
  void test(){
    var script = """
<script>
                            select * from `user` -- use sql comment
                            <where>
                                <if test="true">1=1 -- test</if>
                                <if test="true">and 1=1</if>
                                and 2=2
                            </where>
</script>
      """;
    var languageDriver = new XMLLanguageDriver();
    var sqlSource = languageDriver.createSqlSource(new Configuration(), script, Object.class);
    System.out.println(sqlSource.getBoundSql(null).getSql());
  }

For example, in this comment case, - -test is a comment, and if no newline character exists, the following condition is connected, resulting in the later execution condition to be treated as a comment

-- error sql
select * from `user` -- use sql comment
                             WHERE 1=1 -- test and 1=1 
                                and 2=2

-- 
select * from `user` -- use sql comment
                             WHERE 1=1 -- test
                                and 1=1
                                and 2=2

[nieqiurong]

Can we also expose the shrinkWhitespacesInSql processing method to the users' customization?
The original SqlSourceBuilder.removeExtraWhitespaces does not handle this sql case mentioned above.

System.out.println(SqlSourceBuilder.removeExtraWhitespaces("select * from `user` -- use sql comment\n" +
            "                             WHERE 1=1 -- test\n" +
            "                                and 1=1\n" +
            "                                and 2=2"));

// select * from `user` -- use sql comment WHERE 1=1 -- test and 1=1 and 2=2

[harawata]

Thanks for the comment, @nieqiurong !

In general, you should avoid single line comments when you use JDBC.
Use multiline comments /* comment */ instead.

Still, it may be better not to surprise users.
I give up on #3349 (for now) and will evaluate the effect of this PR.

[nieqiurong]

@harawata Thank you for your reply.

Yes, if treated by removing new lines and adding blank spaces, it will cause some incompatibilities.

<script>
                            select * from `user`
                            <where>
                                <if test="true">1=1</if><if test="true">and 1=1</if>
                                and 2=2
                            </where>
                            <if test="true">and 3=3</if><if test="true">and 4=4</if>
</script>

select * from `user`
                             WHERE 1=1 and 1=1 
                                and 2=2 and 3=3 and 4=4

-- old version  
select * from `user`
                             WHERE 1=1and 1=1
                                and 2=2 
                             and 3=3 and 4=4

Maybe there are better options for improvement, but I haven't found it yet.

[harawata]

@nieqiurong ,

I made a small change. Please take a look and see if everything is OK.

Also, I plan to merge #3422 as it resolve the inconsistency you mentioned in your last comment.
If there is any concern, please let me know.

[nieqiurong]

@harawata
No problem, but I think merging #3422 may not be a good choice as it may not be very compatible

<script>
                            select * from `user`
                            <where>
                                <if test="true">col</if><if test="true">1</if> = #{val}
                            </where>
</script>

expect: select * from user
WHERE col1 = ?
When the dynamic tag is on one line, adding a space will cause interruption, but in lower versions it is connected

Should we consider compatibility?

[harawata]

@nieqiurong ,

There really is no good reason to write conditional SQL that way.
Whatever the conditions are, it can be written without relying on the inconsistent behavior of <where>, <trim> or <set>.

<choose>
<when test="x">col1</when>
<when test="y">col</when>
...
</choose>

Anyway, it is also true that no one complained about the inconsistency so far, so I will leave #3422 open until someone reports it.

Thank you very much for your insight!

[nieqiurong]

Thank you, I think this writing style is also quite extreme This writing style can be ignored during refactoring