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 configuration key "spring.data.web.pageable.serialization-mode" defaults to DIRECT #39797

Open
wants to merge 1 commit into
base: main
Choose a base branch
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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants