Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a configuration property for Spring Data Web's serialization mode #39797

Closed
wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Feb 29, 2024

@quaff
Copy link
Contributor Author

quaff commented Feb 29, 2024

Please hold on until Spring Data Bom 2024.0.0 released.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 29, 2024
@@ -79,4 +82,10 @@ public SortHandlerMethodArgumentResolverCustomizer sortCustomizer() {
return (resolver) -> resolver.setSortParameter(this.properties.getSort().getSortParameter());
}

@Primary // override bean created by @EnableSpringDataWebSupport
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should provide a better way to do this, perhaps requiring a change in Spring Data, as I'd prefer not to have multiple beans of the same type in the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

Copy link
Member

@wilkinsona wilkinsona Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odrotbohm I have a vague recollection that we've already discussed this but I haven't been able to find it.

I guess we could use @ConditionalOnProperty to either auto-configure @EnableSpringDataWebSupport or @EnableSpringDataWebSupport(pageSerializationMode = VIA_DTO). I'm a little hesitant as this approach won't scale as more attributes are added but it would work for now I think.

Copy link
Member

@odrotbohm odrotbohm Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We ultimately want to nudge people into using VIA_DTO at some point, as that's the better way of serializing Page instances by default. We kept the default at DIRECT for 2024.0.0 to not force applications into the new model, potentially causing them to break their APIs and thus not upgrading. That said, if the Boot team was fine with such a change, we could of course already flip the default, with the users being able to re-instantiate the old behavior by explicitly adding @EnableSpringDataWebSupport(pageSerializationMode = DIRECT) to their application.

Adding a configuration property to flip that switch might be nice, but at the same time, I wonder whether having to use the annotation isn't just enough? On the other hand, I can see this being a nice completion of the spring.data.web.pageable.… property namespace.

Different auto-configurations based on the property look good to me. We have no plans to extend to different rendering strategies, as ultimately, we'd like users to use Spring HATEOAS to properly create standardized response types that support linking when rendering Page instances. Thus, the enum is more of a flag to opt either into a safer way of serializing those (current model), or opt into the legacy way should we decide to flip the default.

Copy link
Contributor Author

@quaff quaff Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ConditionalOnProperty is not appropriate here, It's just a flag that should be stick to other pageable settings as possible, not like spring.data.redis.client-type which affect creation of other beans.

I'd prefer to not register SpringDataWebSettings bean if pageSerializationMode is default as SpringDataWebAutoConfiguration did, then the @Primary could be removed, here is my proposal: spring-projects/spring-data-commons#3054

@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 29, 2024
@wilkinsona wilkinsona added this to the 3.3.x milestone Feb 29, 2024
@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Feb 29, 2024
quaff added a commit to quaff/spring-data-commons that referenced this pull request Mar 1, 2024
…ault

allow application to register their own SpringDataWebSettings without @primary

see spring-projects/spring-boot#39797 (comment)
@wilkinsona wilkinsona modified the milestones: 3.3.x, 3.x Apr 22, 2024
@wilkinsona
Copy link
Member

wilkinsona commented Apr 22, 2024

I labelled this as blocked but neglected to say why. We're waiting for the outcome of spring-projects/spring-data-commons#3054.

@odrotbohm
Copy link
Member

It could be that I am missing something here, but the presence of a SpringDataWebSettings is an indication of a user-provided @EnableSpringDataWebSupport annotation. Shouldn't it be possible to simply guard the creation of the SpringDataWebSettings bean here with the presence of one of the ImportBeanDefinitionRegistrars the annotation registers and of course the lack of a SDWS bean definition itself?

It feels a bit weird to, if the annotation is configured, not forward its configuration default value into downstream configuration setup. That extending into having to write that latter code re-implementing a default that's already set and conveyed through the annotation attribute's value.

@wilkinsona
Copy link
Member

Thanks, @odrotbohm.

Adding a configuration property to flip that switch might be nice, but at the same time, I wonder whether having to use the annotation isn't just enough?

As things stand, if you use the annotation, you'll lose our customization of paging and sorting.

Shouldn't it be possible to simply guard the creation of the SpringDataWebSettings bean here with the presence of one of the ImportBeanDefinitionRegistrars the annotation registers.

We already use @EnableSpringDataWebSupport in our auto-configuration when we detect that the user hasn't already used it. Unfortunately, SpringDataWebSettingsRegistar then always registers a SpringDataWebSettings that we can't (easily) replace. We can work around this with two different classes (ignoring fuzzier matching of direct and via-dto:

	@Configuration(proxyBeanMethods = false)
	@EnableSpringDataWebSupport
	@ConditionalOnProperty(name = "spring.data.web.pageable.serialization-mode", havingValue = "direct",
			matchIfMissing = true)
	class EnableSpringDataWebSupportConfiguration {

	}

	@Configuration(proxyBeanMethods = false)
	@EnableSpringDataWebSupport(pageSerializationMode = PageSerializationMode.VIA_DTO)
	@ConditionalOnProperty(name = "spring.data.web.pageable.serialization-mode", havingValue = "via-dto",
			matchIfMissing = false)
	class EnableCustomizedSpringDataWebSupportConfiguration {

	}

spring-projects/spring-data-commons#3054 would remove the need for this little bit of complexity in Boot.

@snicoll
Copy link
Member

snicoll commented Jul 29, 2024

@odrotbohm could you please review the comment above when you get a chance? Thanks!

@snicoll snicoll added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Jul 29, 2024
odrotbohm pushed a commit to spring-projects/spring-data-commons that referenced this pull request Aug 27, 2024
We now refrain from registering a SpringDataWebSettings instance as bean in the ApplicationContext if @EnableSpringDataWebSupport is used without an explicit declaration of pageSerializationMode. This allows Spring Boot to use the annotation, but allow the attribute value to be configured via a property at the same time.

See spring-projects/spring-boot#39797 (comment) for details.

Fixes GH-3054.
odrotbohm pushed a commit to spring-projects/spring-data-commons that referenced this pull request Aug 27, 2024
We now refrain from registering a SpringDataWebSettings instance as bean in the ApplicationContext if @EnableSpringDataWebSupport is used without an explicit declaration of pageSerializationMode. This allows Spring Boot to use the annotation, but allow the attribute value to be configured via a property at the same time.

See spring-projects/spring-boot#39797 (comment) for details.

Fixes GH-3054.
@odrotbohm
Copy link
Member

I'm sorry this took so long to get to. I've merged @quaff's contribution and ported it back into the 3.3.x branch. We now do not register a SpringDataWebSettings bean anymore, unless a non-default value for pageSerializationMode is set. Hope this eases the situation for Boot. Please let me know, if there's anything else we can do.

@philwebb philwebb removed status: blocked An issue that's blocked on an external project change status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Aug 27, 2024
@quaff
Copy link
Contributor Author

quaff commented Aug 28, 2024

@Primary removed now.

@wilkinsona wilkinsona modified the milestones: 3.x, 3.4.x Sep 5, 2024
@wilkinsona wilkinsona added the for: merge-with-amendments Needs some changes when we merge label Sep 5, 2024
@wilkinsona wilkinsona removed the for: merge-with-amendments Needs some changes when we merge label Sep 6, 2024
@wilkinsona wilkinsona self-assigned this Sep 6, 2024
@wilkinsona
Copy link
Member

Thank you, @quaff.

@wilkinsona wilkinsona changed the title Add configuration key "spring.data.web.pageable.serialization-mode" defaults to DIRECT Add a configuration property for Spring Data Web's serialization mode Sep 6, 2024
JKatzwinkel added a commit to JKatzwinkel/tla-es that referenced this pull request Sep 20, 2024
loading the application context fails under spring boot 3.4.0-M3 due to a
`NoUniqueBeanDefinitionException` caused by there being 2
`SpringDataWebSettings` beans available during bean dependency resolution. This
might be related to the changes made in

spring-projects/spring-boot#39797

Anyway, adding the `@EnableSpringDataWebSupport` annotation fixes the problem.
mipo256 pushed a commit to mipo256/spring-data-commons that referenced this pull request Sep 21, 2024
We now refrain from registering a SpringDataWebSettings instance as bean in the ApplicationContext if @EnableSpringDataWebSupport is used without an explicit declaration of pageSerializationMode. This allows Spring Boot to use the annotation, but allow the attribute value to be configured via a property at the same time.

See spring-projects/spring-boot#39797 (comment) for details.

Fixes spring-projectsGH-3054.
izeye added a commit to izeye/spring-boot that referenced this pull request Sep 22, 2024
JKatzwinkel added a commit to JKatzwinkel/tla-es that referenced this pull request Sep 22, 2024
* build(deps): bump the spring-boot group with 5 updates

Bumps the spring-boot group with 5 updates:

| Package | From | To |
| --- | --- | --- |
| [org.springframework.boot:spring-boot-starter-tomcat](https://github.com/spring-projects/spring-boot) | `3.4.0-M2` | `3.4.0-M3` |
| [org.springframework.boot:spring-boot-autoconfigure](https://github.com/spring-projects/spring-boot) | `3.4.0-M2` | `3.4.0-M3` |
| [org.springframework.boot:spring-boot-starter-logging](https://github.com/spring-projects/spring-boot) | `3.4.0-M2` | `3.4.0-M3` |
| [org.springframework.boot:spring-boot-starter-test](https://github.com/spring-projects/spring-boot) | `3.4.0-M2` | `3.4.0-M3` |
| [org.springframework.boot](https://github.com/spring-projects/spring-boot) | `3.4.0-M2` | `3.4.0-M3` |


Updates `org.springframework.boot:spring-boot-starter-tomcat` from 3.4.0-M2 to 3.4.0-M3
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.4.0-M2...v3.4.0-M3)

Updates `org.springframework.boot:spring-boot-autoconfigure` from 3.4.0-M2 to 3.4.0-M3
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.4.0-M2...v3.4.0-M3)

Updates `org.springframework.boot:spring-boot-starter-logging` from 3.4.0-M2 to 3.4.0-M3
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.4.0-M2...v3.4.0-M3)

Updates `org.springframework.boot:spring-boot-starter-test` from 3.4.0-M2 to 3.4.0-M3
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.4.0-M2...v3.4.0-M3)

Updates `org.springframework.boot` from 3.4.0-M2 to 3.4.0-M3
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.4.0-M2...v3.4.0-M3)

---
updated-dependencies:
- dependency-name: org.springframework.boot:spring-boot-starter-tomcat
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: spring-boot
- dependency-name: org.springframework.boot:spring-boot-autoconfigure
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: spring-boot
- dependency-name: org.springframework.boot:spring-boot-starter-logging
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: spring-boot
- dependency-name: org.springframework.boot:spring-boot-starter-test
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: spring-boot
- dependency-name: org.springframework.boot
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: spring-boot
...

Signed-off-by: dependabot[bot] <support@github.com>

* fix spring data web support autoconfiguration

loading the application context fails under spring boot 3.4.0-M3 due to a
`NoUniqueBeanDefinitionException` caused by there being 2
`SpringDataWebSettings` beans available during bean dependency resolution. This
might be related to the changes made in

spring-projects/spring-boot#39797

Anyway, adding the `@EnableSpringDataWebSupport` annotation fixes the problem.

* ci: add java 23 to test matrix

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: JKatzwinkel <JKatzwinkel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants