spring-projects / spring-hateoas

Spring HATEOAS - Library to support implementing representations for hyper-text driven REST web services.
https://spring.io/projects/spring-hateoas
Apache License 2.0
1.03k stars 475 forks source link

0.23.0 problem with link generation with null values #530

Closed johann-sonntagbauer closed 7 years ago

johann-sonntagbauer commented 7 years ago
/* Controller */

@RequestMapping(value = "/{entityContext}/{searchSpecId}/{formLayoutId}", method = RequestMethod.GET, produces = {MediaType.APPLICATION_JSON_UTF8_VALUE})
    public HttpEntity<EntityTable> executeFilterByFormLayout(@PathVariable String entityContext,
            @PathVariable String searchSpecId,
            @PathVariable String formLayoutId,
            @RequestParam(required = false) String eql,
            @RequestParam(required = false) Integer maxResults,
            @RequestParam(required = false) Integer lastIndex,
            @RequestParam(required = false, defaultValue = "false") Boolean counting) {
        ...
    }
/* Link Generation */

  linkTo(
    methodOn(SearchFXApiController.class)
      .executeFilterByFormLayout(resource.getEntityContext(), resource.getSearchSpec(), formLayoutId, null, null, null, null)
  ).withRel(REL_SUBMIT_FORM);

results in following link: http://localhost:8080/api/v1/fx/search/course/COURSEGUIDE_LEARNING/DEFAULT{?eql,maxResults,lastIndex,counting}

It seems that all null values are collected within the {?} block at the end

johann-sonntagbauer commented 7 years ago

Added an minimal breaking testcase in the testsuite. Please take a look at the referenced commit.

otrosien commented 7 years ago

I think this is intended, as Link internally holds an UriTemplate. use expand() if you want to pre-fill the values. The methodOn signature should be seen as "provide the necessary parameter-types to uniquely identify which method you are referring to", not a way to pass the real parameter values down to the Link.

johann-sonntagbauer commented 7 years ago

hm could be. The current situation with the api is kind of confusing. I simply thought of this beeing a bug, because the api was working with 0.21 and broke during the update.

johann-sonntagbauer commented 7 years ago

@otrosien You are on to something :) I've added link = link.expand(); to the testcase and now it works. But the situation is kind a wired because the api doesn't reflect if I get an UriTemplate or an final Uri. Is the best practicse to always call link.expand() ?

odrotbohm commented 7 years ago

A Link never made a guarantee the href is a URI. That's why getHref() doesn't return URI in the first place. The effect you see is a side effect of our added support of partial URI template expansion on ControllerLinkBuilder. With that in place, dropping unexpanded request parameters is not an option as it'd create inconsistent URI templates with some parameters kept and some dropped.

Link.expand() still keeps the behavior it always had: rejecting non-optional template parameters (like e.g. path segments) and dropping request parameter template variables.

johann-sonntagbauer commented 7 years ago

@olivergierke Thank you for the advice. I have not found any hint about that behaviour in the documentation. Maybe that point should be made more obviouse in the docs with an example.

johann-sonntagbauer commented 7 years ago

so the pattern to use should be

@Test
public void linksToMethodWithPathVariableAndParameter() {
  Link link = linkTo(methodOn(ControllerWithMethods.class).methodForNextPage(null, null, null)).withSelfRel();
  assertPointsToMockServer(link);

  Map<String, Object> args = new HashMap<String, Object>();
  args.put("id", 12);
  args.put("limit", 150);
  link = link.expand(args);
  assertThat(link.getHref(), endsWith("/something/12/foo?limit=150"));
}
otrosien commented 7 years ago

I actually find it quite handy to give out templated URIs, esp. with regards to paging, but you need to make sure all your clients support RFC 6570.

johann-sonntagbauer commented 7 years ago

@otrosien Cool, never heard of the RFC but we definitly will use it. Thank you very much.