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

Introduce argument resolver for OffsetScrollPosition #2856

Closed
quaff opened this issue Jun 16, 2023 · 3 comments
Closed

Introduce argument resolver for OffsetScrollPosition #2856

quaff opened this issue Jun 16, 2023 · 3 comments
Labels
status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement

Comments

@quaff
Copy link
Contributor

quaff commented Jun 16, 2023

Just like org.springframework.data.web.PageableHandlerMethodArgumentResolver did for Pageable

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 16, 2023
@mp911de
Copy link
Member

mp911de commented Jun 16, 2023

We do not have a good way to capture the keyset type. For offset-based positions, we could provide something. Feel free to submit a pull request. We should provide argument resolves for the servlet- and the reactive stack.

@mp911de mp911de added status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 16, 2023
@quaff
Copy link
Contributor Author

quaff commented Jun 16, 2023

I will try OffsetScrollPosition first.

quaff added a commit to quaff/spring-data-commons that referenced this issue Jun 16, 2023
quaff added a commit to quaff/spring-data-commons that referenced this issue Jun 16, 2023
quaff added a commit to quaff/spring-data-commons that referenced this issue Jun 17, 2023
quaff added a commit to quaff/spring-data-commons that referenced this issue Jun 17, 2023
@quaff
Copy link
Contributor Author

quaff commented Jun 19, 2023

For KeysetScrollPosition:

Given

@GetMapping("/users")
public Window<User> scroll(@SortDefault(sort = "id", direction = ASC) Sort sort,
		@RequestParam(defaultValue = "10") int limit, KeysetScrollPosition position) {
	Specification<User> spec = (root, query, cb) -> null;
	return this.userRepository.findBy(spec, q -> q.limit(limit).sortBy(sort).scroll(position));
}

Use implicit id as sort:

/users -> ScrollPosition.forward(Map.of())
/users?id=10 -> ScrollPosition.forward(Map.of("id", 10))
/users?direction=backward -> ScrollPosition.backward(Map.of())
/users?direction=backward&id=10 -> ScrollPosition.backward(Map.of("id", 10))

Use explicit username as sort:

/users?sort=username -> ScrollPosition.forward(Map.of())
/users?sort=username&username=test -> ScrollPosition.forward(Map.of("username", "test"))
/users?sort=username&direction=backward -> ScrollPosition.backward(Map.of())
/users?sort=username&direction=backward&username=test -> ScrollPosition.backward(Map.of("username", "test"))

Use explicit foo and bar combination as sort:

/users?sort=foo,bar -> ScrollPosition.forward(Map.of())
/users?sort=foo,bar&foo=foo&bar=bar -> ScrollPosition.forward(Map.of("foo", "foo", "bar", "bar"))
/users?sort=foo,bar&direction=backward -> ScrollPosition.backward(Map.of())
/users?sort=foo,bar&direction=backward&foo=foo&bar=bar -> ScrollPosition.backward(Map.of("foo", "foo", "bar", "bar"))

The key problem is how to convert keys to correct type, maybe we should introduce an annotation to extract actual type from underlying domain object, for example

@TypeHints(from = User.class) KeysetScrollPosition position

WDYT? @mp911de

quaff added a commit to quaff/spring-data-commons that referenced this issue Jun 19, 2023
@mp911de mp911de linked a pull request Jul 4, 2023 that will close this issue
@mp911de mp911de changed the title Implement argument resolver for ScrollPosition Introduce argument resolver for OffsetScrollPosition Jul 4, 2023
@mp911de mp911de added this to the 3.2 M1 (2023.1.0) milestone Jul 4, 2023
@mp911de mp911de closed this as completed in 391607b Jul 4, 2023
mp911de added a commit that referenced this issue Jul 4, 2023
Allow nullable OffsetScrollPosition and add support for values wrapped within Optional.

Update tests. Reformat code. Add issue tags to tests.

See #2856
Original pull request: #2861
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants