Skip to content

PagedResourcesAssembler generated navigation links ignore non pageable request parameters [DATACMNS-1757] #2173

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

Closed
spring-projects-issues opened this issue Jun 28, 2020 · 2 comments
Assignees
Labels
in: mapping Mapping and conversion infrastructure type: bug A general bug

Comments

@spring-projects-issues
Copy link

Réda Housni Alaoui opened DATACMNS-1757 and commented

Let's consider the following test:

@Test
  @DisplayName("GET with pageable and additional request parameter")
  public void test2() throws Exception {
    mockMvc
        .perform(
            MockMvcRequestBuilders.get("/api/PagedResourcesAssemblerTest")
                .queryParam("text", "foo")
                .queryParam("page_size", "1")
                .queryParam("page_number", "1"))
        .andExpect(status().isOk())
        .andExpect(jsonPath("$._links.first.href").value(containsString("text=foo")));
  }

@Controller
  @RequestMapping("/PagedResourcesAssemblerTest")
  public static class MyController {

    @GetMapping
    public ResponseEntity<?> get(
        @RequestParam(name = "text", required = false) String text,
        Pageable pageable,
        PagedResourcesAssembler<Element> pageAssembler) {
      Page<Element> page =
          new PageImpl<>(Collections.singletonList(new Element("foo")), pageable, 10);
      return ResponseEntity.ok(pageAssembler.toModel(page));
    }
  }

The described test fails because the generate response body is:

{
  "_embedded": {
    "content": [{
      "name": "foo"
    }]
  },
  "_links": {
    "first": {
      "href": "http://localhost/api/PagedResourcesAssemblerTest?page_number=0&page_size=1"
    },
    "prev": {
      "href": "http://localhost/api/PagedResourcesAssemblerTest?page_number=0&page_size=1"
    },
    "self": {
      "href": "http://localhost/api/PagedResourcesAssemblerTest?page_number=1&page_size=1"
    },
    "next": {
      "href": "http://localhost/api/PagedResourcesAssemblerTest?page_number=2&page_size=1"
    },
    "last": {
      "href": "http://localhost/api/PagedResourcesAssemblerTest?page_number=9&page_size=1"
    }
  },
  "page": {
    "size": 1,
    "totalElements": 10,
    "totalPages": 10,
    "number": 1
  }
}

The navigation links hold the correct page parameters, but they are missing request parameter text.


Affects: 2.4 M1 (2020.0.0)

Referenced from: pull request #452

@spring-projects-issues spring-projects-issues added type: bug A general bug in: mapping Mapping and conversion infrastructure labels Dec 30, 2020
@odrotbohm
Copy link
Member

I am inclined to go down a slightly different route than what's suggested in #452. Here's what's actually causing the issue: when PagedResourcesAssembler is resolved for method injection, our HanderMethodArgumentResolver currently equips it with a default URI that is the one created when pointing to the method that is currently invoked. In the course of that, optional request parameters are dropped as we don't consider the request parameters when expanding those potentially optional parameters.

That said, I think wen can just skip the pre-computation entirely as PagedResourcesAssembler will fall back to use the current request's URI as the base for the links to be created. If controller methods want to use anything but that, they're free to calculate that URI and hand a Link into an overload of ….toModel(…) explicitly. I just briefly checked whether that removal works and except a test case checking for some internal state of PRA no test cases fail, also not in Spring Data REST.

I am inclined to back-port this into 2.4 (i.e. Spring Boot 2.4) as well as it's a rather young branch and we can easily tweak things back in case this causes trouble. For Neumann (SR7 approaching) I'd leave the decision up for @mp911de.

@odrotbohm odrotbohm added this to the 2.3.7 (Neumann SR7) milestone Jan 26, 2021
odrotbohm added a commit that referenced this issue Jan 26, 2021
We now avoid the pre-computation of the base URI in PagedResourceAssemblerArgumentResolver as the actual assembler will fall back to using the URI of the current request by default anyway. The latter makes sure that request parameters, that are contained in the original requests appear in links created. If a controller wants to deviate from that behavior, they can create a dedicated link themselves and hand that to the assembler explicitly.

Fixes GH-2173, GH-452.
odrotbohm added a commit that referenced this issue Jan 26, 2021
We now avoid the pre-computation of the base URI in PagedResourceAssemblerArgumentResolver as the actual assembler will fall back to using the URI of the current request by default anyway. The latter makes sure that request parameters, that are contained in the original requests appear in links created. If a controller wants to deviate from that behavior, they can create a dedicated link themselves and hand that to the assembler explicitly.

Fixes GH-2173, GH-452.
@reda-alaoui
Copy link
Contributor

Thank you @odrotbohm !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: mapping Mapping and conversion infrastructure type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants