Document "SpEl support in @Query" feature as a breaking change

MaartenTutak commented 2 months ago

Hi,

I just noticed one of our integration tests failing after upgrading Spring Boot from 3.2.6 to 3.3.1 It seems #2826 (intentionally) added a breaking change regarding the handling of null values in the context of the @Query annotation. While I have no specific opinion on this change, I did notice it is not documented as a breaking change on any resource I can find. In our case specifically, we went from receiving an HTTP 200 response from ES containing no hits to having a ConversionException thrown.

Kind regards, Maarten Tutak

sothawo commented 2 months ago

Hm, thanks for this hint. Can you provide an example of what worked before and is breaking now. Not sure, if this behavioural change should have been.

MaartenTutak commented 2 months ago

Hi,

sure thing.

https://github.com/MaartenTutak/spring-data-elasticsearch-2936/tree/spring-boot-3.2.6 https://github.com/MaartenTutak/spring-data-elasticsearch-2936/tree/spring-boot-3.3.1

FooRepositoryIntegrationTest works on 3.2.6, but not on 3.3.1

Thanks, Maarten

sothawo commented 2 months ago

Thank you for the sample code.

You are right, this breaking change should have been documented, we missed that. I updated the migration guide from version 5.2 to 5.3 to include this information (8d0ecf2aa38526412d3aa9e464d3cdbf857f0c71).

As for the reason to change that behaviour: Up to version 5.2 when a null parameter was encountered this was replaced with the string "null" in the query. Your test with Boot 3.2.6 would fail as well if you change your setup to:

    @BeforeEach
    void setUp() {
        fooRepository.save(new Foo(UUID.randomUUID(), "Foo", "null"));
    }

because now the search fooRepository.findBy("Foo", null) would return the one entity stored in the index. This only makes sense when "null" was defined as the null_value in the Elasticsearch field mapping (which it isn't in your case). And to search for the null_value now the user needs to explicitly call fooRepository.findBy("Foo", "null").

Further docs about null values: https://www.elastic.co/guide/en/elasticsearch/guide/current/_dealing_with_null_values.html

puppylpg commented 2 months ago

This situation was stated in #2833 :

Another problem is that when using placeholder(?0, ?1...) in @Query, null value will be converted to string "null", and this should be considered a bug as stated in https://github.com/spring-projects/spring-data-elasticsearch/pull/2826#discussion_r1454807556. The simplest way to deal with that is to use the new ConversionService in StringQueryUtil, since the conversion logic has already been well handled in ConversionService.

propose2: replece the old logic for values conversion in StringQueryUtil;

It should have been added into breaking-change docs. 👍

spring-projects / spring-data-elasticsearch

Document "SpEl support in @Query" feature as a breaking change #2936