Made JSqlParserQueryEnhancer aware of MERGE statements.

[gderemetz]

Related tickets #2641

• You have read the Spring Data contribution guidelines.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.
• You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

[DiegoKrupitza]

I like your extension :) It should resolve the problem. Please review my comments. The JavaDoc is important, so the documentation stays up-to-date. Regarding the alias I would love to hear your take on this.

[DiegoKrupitza]

Please add the new MERGE to the JavaDoc of the enum.

<li>{@code ParsedType.MERGE}: means the top level statement is {@link Merge}</li>

[DiegoKrupitza]

I don't know if this is ever needed but as far as I can see a merge has an "alias" (see screenshot).
Maybe it should not return NULL here. Is it possible to adapt the detectAlias() to consider this edge case without making the method overly complicated?

[gderemetz]

I tried to adapt the detectAlias() method for JSqlParserQueryEnhancer class, however it is more difficult for me to modify the method for the DefaultQueryEnhancer (82590bf)
I am waiting your feedback

[DiegoKrupitza]

Yes your are totally right! It seems that the DefaultQueryEnhancer (which uses QueryUtils) does not correctly detect the alias, when providing a Merge statement. I checked this using the following test:

@Test 
void queryUtilsDetectsMergeAliasCorrectly() {
String alias = QueryUtils.detectAlias(
		"merge into a using (select id, value from b) query on (a.id = query.id) when matched then update set a.value = value");

assertThat(alias).isNotNull().isNotEmpty();
}

I think for this we have to turn to @gregturn or any other member of the team.
Do we want the JSqlParserQueryEnhancer to "simulate" the same behaviour as DefaultQueryEnhancer (which is at the moment returning smth wrong aka NULL for a Merge statement) or should it be returning the actual correct alias.

Personally I would like JSqlParserQueryEnhancer to return the correct alias. Ergo stick with your implementation.

If the members are also of the same opinion, we should, in later stages (another PR), make DefaultQueryEnhancer (entails QueryUtils) aware of the fact that a merge statement can have an alias.

[DiegoKrupitza]

I have seen this is your first PR here, welcome to Open Source 🥳👍 While reviewing your PR I have seen your forgot to add the Closes #2641 to your commit. Adding this is quite helpful because then GitHub then create automatic links between commits and issues.

https://github.com/spring-projects/spring-data-build/blob/main/CONTRIBUTING.adoc#commit-messages

[DiegoKrupitza]

All in all it looks good. I just left two comments just for personal taste.

[DiegoKrupitza]

Is there a reason why you decided to make the method static?

[gderemetz]

I just kept the same behavior for the function just above (static method) to have a homogeneous code :
private static String detectAlias(PlainSelect selectBody)

[DiegoKrupitza]

Oh makes sense! I can't remember anymore why I made that method static 😅

[gderemetz]

I made a force push ... I hope this is not a mistake

[DiegoKrupitza]

Thats a clever solution! Makes it more flexible in the future.
Personally I would also add a "default" version where we know it will be a Select. Because as you can see above we often call parseSelectStatement(query.getQueryString(), Select.class);.

       /**
	 * Parses a query string with JSqlParser.
	 *
	 * @param query the query to parse
	 * @return the parsed query
	 */
	private Select parseSelectStatement(String query) {
		return parseSelectStatement(query,Select.class);
	}

[gregturn]

Thanks to @xercasy and @DiegoKrupitza! I have polished and merged this PR to main.