search

GoogleCloudPlatform / spring-cloud-gcp

New home for Spring Cloud GCP development starting with version 2.0.
Apache License 2.0
399 stars 295 forks source link

Spring Boot 3.3 upgrade causes serialization loss of DatastorePageable.urlSafeCursor #2940

Closed burkedavison closed 1 month ago

burkedavison commented 1 month ago

@odrotbohm , could you verify that the below understanding is correct? Do you have any recommendations to resolve this issue?

PR https://github.com/GoogleCloudPlatform/spring-cloud-gcp/pull/2923 shows the following test failure when updating from Spring Boot 3.2 to 3.3:

Error: 2:038 [ERROR] com.example.DatastoreBookshelfExampleIntegrationTests.testSerializedPage -- Time elapsed: 0.474 s <<< FAILURE!
java.lang.AssertionError: 

Expecting actual:
  "***"content":[***"id":12345678***],"page":***"size":1,"number":0,"totalElements":4,"totalPages":4***"
to contain pattern:
  ""pageable":.+"urlSafeCursor":".+""

    at com.example.DatastoreBookshelfExampleIntegrationTests.testSerializedPage(DatastoreBookshelfExampleIntegrationTests.java:82)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

The test is verifying that urlSafeCursor (defined in Spring Cloud GCP's DatastorePageable) is part of the response body. I've tracked the change in behavior down to Spring Data's new registration of PagedModel which is now used when serializing a PageImpl with Jackson. This PageModel class produces the serialized page entry in the above 'actual' test results. This change was made "to make sure the representations stay stable and do not expose unnecessary implementation details".

I believe the intent was to introduce this new converter as opt-in only to preserve backward compatibility, based on the documentation for this change

Configures how to render PageImpl instances. Defaults to PageSerializationMode#DIRECT for backward compatibility reasons.

and the commit message

@EnableSpringDataWeb support now contains a pageSerializationMode attribute set to an enum with two possible values: DIRECT, which is the default for backwards compatibility reasons.

However, the logic determining the activation of this converter is only backwardly compatible for users of the @EnableSpringDataWebSupport annotation. If the annotation is not being used, the default behavior is to now use the PagedModel converter. 

if (settings != null // When using the @EnableSpringDataWebSupport annotation...
    && settings.pageSerializationMode() == PageSerializationMode.DIRECT) 
{
    setMixInAnnotation(PageImpl.class, WarningMixing.class);
} else {
    // When not using the @EnableSpringDataWebSupport annotation...
    setMixInAnnotation(PageImpl.class, WrappingMixing.class);
}
// ...
@JsonSerialize(converter = PlainPageSerializationWarning.class)
abstract class WarningMixing {}

@JsonSerialize(converter = PageModelConverter.class)
abstract class WrappingMixing {} // This class uses the new PageModelConverter.

I believe the intent was for the logic to be:

if (settings == null || settings.pageSerializationMode() == PageSerializationMode.DIRECT) {
    // Use the PlainPageSerializationWarning converter
} else {
    // Use the PageModelConverter
}

Add'l References

odrotbohm commented 1 month ago

You're correct, and we can certainly consider this a bug. I'll have to reach out to the Boot team to find out why the current guard is not triggered. Spring Boot's autoconfiguration for Spring Data's web support actually carries @EnableSpringDataWebSupport.

I've created spring-projects/spring-data-commons#3101. We should be able to get a fix for this into the next service releases (in T-2w).

odrotbohm commented 1 month ago

Fixes should be available in both the 3.3.1 and 3.4.0 snapshots.

burkedavison commented 1 month ago

Thank you for the fast resolution, @odrotbohm !