spring-hateoas icon indicating copy to clipboard operation
spring-hateoas copied to clipboard

linkTo_methodOn not working on GET requested object parameter

Open khong07 opened this issue 9 years ago • 14 comments
trafficstars

Suppose we have this method

@RequestMapping(value= "list")
public HttpEntity<ListResource> getList(
        @RequestParam(value = "text1", required = false) String text1,
        @RequestParam(value = "text2", required = false) String text2,
        @RequestParam(value = "text3", required = false) String text3,
        ...
        @RequestParam(value = "page", defaultValue = "1") Integer page,
        @RequestParam(value = "limit", defaultValue = "10") Integer limit) {
    // ...
}

Because we have a lot of parameters, we rewrote

@RequestMapping(value= "list")
public HttpEntity<ListResource> getList(RequestObject requestObject) {
    // ...
}

public class RequestObject {
    private String text1;
    private String text2;
    private String text3;
    ...
    private Integer page;
    private Integer limit;
}

Spring MVC maps automatically the RequestObject with given parameters

But when generating the Link

final RequestObject requestObject = new RequestObject();
requestObject.setPage(1);
requestObject.setLimit(10);
requestObject.setText1("test");
return ControllerLinkBuilder.linkTo(ControllerLinkBuilder.methodOn(ABCController.class)
                .getList(requestObject));

That doesn't return

http://localhost:8080/list?page=1&limit=10&text1=test
but only
http://localhost:8080/list

do you have any idea?

khong07 avatar Sep 22 '16 14:09 khong07

You don't actually call the method you show above. The method takes a lot of Strings, you hand in a RequestObject. Currently only parameters with @PathVariable or @RequestMapping are supported.

odrotbohm avatar Sep 22 '16 14:09 odrotbohm

@olivergierke Hi, i would like to use the method that takes RequestObject, not the one taking a lot of String. I know that's not supported for now. Do you have any plan to support it like Spring MVC ?(RequestObject)

khong07 avatar Sep 22 '16 15:09 khong07

@khong07 pagination should be handled using a Pageable, maybe that would already reduce the number of parameters to a manageable amount?

If you dig deeper, there might even be a chance of implementing something yourself by implenting a UriComponentsContributor that handles your RequestObject, and registering it at ControllerLinkBuilderFactory. I haven't tried it, but it looks doable.

otrosien avatar Dec 22 '16 21:12 otrosien

@otrosien my implementation still has a lot aof parameters :(

Yes, i think it's possible, thank for suggestion.

khong07 avatar Jan 04 '17 13:01 khong07

I would also welcome the support for RequestObjects because search APIs often use a variety of parameters. This is definitely cleaner to solve with RequestObjects. It also just became clear after a bunch few failed attempts to create proper links, that the support is still missing here.

Example:

@RequestMapping(value= "/search", method = RequestMethod.GET, produces = MediaType.APPLICATION_JSON_UTF8_VALUE)
public ResponseEntity<PagedResources> search(SearchRequest searchRequest, Pageable pageable, PagedResourceAssembler pageAssembler) {
    
  Page<SearchResult> pages = searchService.search(searchRequest);
  Link link = ControllerLinkBuilder.linkTo(methodOn(Controller.class).search(searchRequest, pageable, pageAssembler).withSelfRel();

  return ResponseEntity.ok(pageAssembler.toResource(pages, link));
}

public class RequestObject {
    private String param1;
    private String param2;
    private String param3;
    ...
    private String paramN;
}

Request like GET /search?param1=foo&param2=bar&param3=darth&...&paramN=vader&sort=param1,asc should then produce e.g. a result like http://localhost:8080/search?param1=foo&param2=bar&param3=darth&...&paramN=vader&page=0&size=20&sort=param1,asc

But 2 issues here:

  • missing support for RequestObjects
  • missing expansion of passed Link param with pageable-related data (page, size, sort)

jrauschenbusch avatar Feb 18 '18 13:02 jrauschenbusch

Ok, i found a rough-and-ready workaround: PagedResourcesAssembler pagedResourcesAssembler = new PagedResourcesAssembler(new HateoasPageableHandlerMethodArgumentResolver(), null);

By initializing the PagedResourcesAssembler with null as the baseUri, the query params will not get ignored by the assembler.

I know this is definitively not the intended way to use it, but for now it's fine for me. The generated links are now correct. The only downside is that non-mapped query params are also present and won't get filtered out.

jrauschenbusch avatar Feb 20 '18 18:02 jrauschenbusch

I've found another solution to get a single part of the desired behavior. Now the links are rendered correctly.

Just created my own ControllerLinkBuilderFactory and re-used existing functionality. This was already mentioned by @otrosien. Using a request object is still not possible, but the param list would be anyway wrong if you are trying to use snake_case names as Jackson annotations (attribute naming) are not considered currently as replacement for e.g. @RequestParam(value="param_1").

Here is the relevant code snippet:

ControllerLinkBuilderFactory controllerLinkBuilderFactory = new ControllerLinkBuilderFactory();
controllerLinkBuilderFactory.setUriComponentsContributors(Arrays.asList(new HateoasPageableHandlerMethodArgumentResolver()));
Link link = linkTo(methodOn(Controller.class).search(param1, param2, ..., paramN, pageable, pagedResourcesAssembler)).withSelfRel();

PagedResources pagedResources = pagedResourcesAssembler.toResource(page, link.expand());

jrauschenbusch avatar Mar 16 '18 18:03 jrauschenbusch

Any updates on this issues? I still struggle with the same problem like @khong07

tobspeed avatar Aug 08 '18 20:08 tobspeed

We faced this issue on our project and created a workaround - it's fairly specific to our project, but perhaps it could inspire others: https://gist.github.com/bob983/b26f7af740c6aa0c4913aec43b209c06

Please note that the project uses RxJava and thus uses injected UriComponentsBuilder because Spring Hateoas was trying to get hold of current request which wasn't really working :-)

bob983 avatar Nov 14 '18 11:11 bob983

any update on this? We are facing the same issue and would be grateful for a fix!

evainga avatar Jul 19 '21 08:07 evainga

The reason for the glacial pace here is that there's no real way for us to tell which of the arguments in something like this:

search(SearchRequest searchRequest, Pageable pageable, PagedResourceAssembler pageAssembler) { … }

is one that actually binds request parameters. In fact, SearchRequest is not bound via implicit @RequestParam but via the implicit @ModelAttribute which is a bit unusual use in REST APIs as they usually switch to using the request payload for information transfer. We can definitely investigate what it would mean to also support handler method parameters explicitly annotated with @ModelAttribute but it'll certainly add quite a bit of complexity (how to handle complex properties declared in the type that maps the parameters? etc.) and I am not sure what that's doing to our affordances calculation etc.

odrotbohm avatar Jul 20 '21 12:07 odrotbohm

Hi @odrotbohm ,

We can definitely investigate what it would mean to also support handler method parameters explicitly annotated with @ModelAttribute but it'll certainly add quite a bit of complexity (how to handle complex properties declared in the type that maps the parameters? etc.) and I am not sure what that's doing to our affordances calculation etc.

I suppose that implementing a clean solution would involve modifications to Spring MVC, therefore it would take some time to get it released.

Couldn't we provide some basic support in Spring HATEOAS in the meantime ? For example, Spring HATEOAS could limit its support to nested attribute of type java.lang.String, java.lang.Number and java.lang.Iterable of the two first types.

I implemented a org.springframework.hateoas.server.mvc.UriComponentsContributor for that. I wanted to support immutable @ModelAttribute objects. The main difficulty for me was to make sure that my UriComponentsContributor could parse the correct parameters values of an already built SearchRequest without knowing which constructor implementation was previously used. Let's imagine the following setup:

class SearchRequest {
  private final String myFoo;
  private final String myBar;

  @ConstructorProperties({"my_foo", "my_bar"})
  SearchRequest(String myFoo, String myBar) {
    this.myFoo = myFoo;
    this.myBar = myBar;
  }

  public String getMyFoo() {
    return myFoo;
  }

  public String getMyBar() {
    return myBar;
  }

  @Override
  public boolean equals(Object o) {
   // return o.myFoo==this.myFoo && o.myBar == this.myBar;
  }

}
@Test
void test() {
    assertThat(
            webMvcLinkBuilderFactory
                .linkTo(methodOn(MyController.class).echo(new SearchRequest("one", "two")))
                .toUri())
        .hasToString("http://localhost/list?my_foo=one&my_bar=two");
  }

To enhance UriComponentsBuilder, my UriComponentsContributor would have to make sure that constructor argument myFoo is directly assigned to attribute this.myFoo and myBar is directly assigned to attribute this.myBar to deduce my_foo=one and my_bar=two. I believe you can't have this guarantee with java classes. Someone could just do that:

class SearchRequest {
  private final String myFoo;
  private final String myBar;

  @ConstructorProperties({"my_foo", "my_bar"})
  SearchRequest(String myFoo, String myBar) {
    this.myFoo = myFoo.substring(1);
    this.myBar = myBar.substring(1);
  }

  public String getMyFoo() {
    return myFoo;
  }

  public String getMyBar() {
    return myBar;
  }

  @Override
  public boolean equals(Object o) {
   // return o.myFoo==this.myFoo && o.myBar == this.myBar;
  }
}

You would end up with my_foo=ne and my_bar=wo which would lead to "e".equals(searchRequest.getMyBar()) and "o".equals(searchRequest.getMyFoo()) on http request processing. TLDR: the instance passed to UriComponentsContributor would not be equal to the instance created by Spring MVC during http request processing.

I believe the solution to this issue was introduced with Java Record. Its specification has the following requirement:

For all record classes, the following invariant must hold: if a record R's components are c1, c2, ... cn, then if a record instance is copied as follows: R copy = new R(r.c1(), r.c2(), ..., r.cn());
then it must be the case that r.equals(copy).

So I think a basic solution can be implemented in version 2 since it supports Java 17 at source level.

Here is my UriComponentsContributor implementation:

import java.beans.ConstructorProperties;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.RecordComponent;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
import org.springframework.beans.BeanUtils;
import org.springframework.core.MethodParameter;
import org.springframework.hateoas.TemplateVariable;
import org.springframework.hateoas.TemplateVariables;
import org.springframework.hateoas.server.mvc.UriComponentsContributor;
import org.springframework.stereotype.Component;
import org.springframework.web.method.annotation.ModelAttributeMethodProcessor;
import org.springframework.web.util.UriComponents;
import org.springframework.web.util.UriComponentsBuilder;

/** @author Réda Housni Alaoui */
@Component
class ModelAttributeRecordUriContributor implements UriComponentsContributor {

  private final ModelAttributeMethodProcessor modelAttributeMethodProcessor =
      new ModelAttributeMethodProcessor(true);

  @Override
  public boolean supportsParameter(MethodParameter parameter) {
    return modelAttributeMethodProcessor.supportsParameter(parameter)
        && parameter.getParameterType().isRecord();
  }

  @Override
  public void enhance(UriComponentsBuilder builder, MethodParameter parameter, Object record) {
    if (parameter == null || record == null) {
      return;
    }

    Class<?> recordType = parameter.getParameterType();

    Constructor<?> canonicalConstructor = fetchCanonicalConstructor(recordType);

    String[] parameterNames = BeanUtils.getParameterNames(canonicalConstructor);
    RecordComponent[] recordComponents = recordType.getRecordComponents();
    for (int constructorParamIndex = 0;
        constructorParamIndex < recordComponents.length;
        constructorParamIndex++) {
      RecordComponent recordComponent = recordComponents[constructorParamIndex];
      Object queryParamValue;
      try {
        queryParamValue = recordComponent.getAccessor().invoke(record);
      } catch (IllegalAccessException | InvocationTargetException e) {
        throw new RuntimeException(e);
      }
      Object[] values = parseQueryParamValues(queryParamValue).orElse(null);
      if (values == null) {
        continue;
      }
      builder.queryParam(parameterNames[constructorParamIndex], values);
    }
  }

  @Override
  public TemplateVariables enhance(
      TemplateVariables templateVariables, UriComponents uriComponents, MethodParameter parameter) {

    Class<?> recordType = parameter.getParameterType();
    Constructor<?> canonicalConstructor = fetchCanonicalConstructor(recordType);
    String[] parameterNames = BeanUtils.getParameterNames(canonicalConstructor);

    TemplateVariable.VariableType variableType;
    if (uriComponents.getQueryParams().isEmpty()) {
      variableType = TemplateVariable.VariableType.REQUEST_PARAM;
    } else {
      variableType = TemplateVariable.VariableType.REQUEST_PARAM_CONTINUED;
    }

    Collection<TemplateVariable> variables =
        Arrays.stream(parameterNames)
            .map(variableName -> new TemplateVariable(variableName, variableType))
            .toList();

    return templateVariables.concat(variables);
  }

  private Constructor<?> fetchCanonicalConstructor(Class<?> recordType) {
    RecordComponent[] recordComponents = recordType.getRecordComponents();
    if (recordComponents == null) {
      throw new IllegalArgumentException("%s is not a record !".formatted(recordType));
    }

    Class[] parameterTypes =
        Arrays.stream(recordComponents).map(RecordComponent::getType).toArray(Class[]::new);

    Constructor<?> canonicalConstructor;
    try {
      canonicalConstructor = recordType.getDeclaredConstructor(parameterTypes);
    } catch (NoSuchMethodException e) {
      throw new RuntimeException(e);
    }

    List<Constructor<?>> annotatedConstructors =
        Arrays.stream(recordType.getDeclaredConstructors())
            .filter(constructor -> constructor.isAnnotationPresent(ConstructorProperties.class))
            .toList();

    if (annotatedConstructors.size() > 1) {
      throw new RuntimeException(
          "%s has multiple constructors annotated with %s. Only the record canonical constructor can be annotated with %s."
              .formatted(recordType, ConstructorProperties.class, ConstructorProperties.class));
    }

    Constructor<?> singleAnnotatedConstructor =
        annotatedConstructors.stream().findFirst().orElse(null);
    if (singleAnnotatedConstructor != null
        && !singleAnnotatedConstructor.equals(canonicalConstructor)) {
      throw new RuntimeException(
          "%s has a non canonical constructor annotated with %s. Only the record canonical constructor can be annotated with %s."
              .formatted(recordType, ConstructorProperties.class, ConstructorProperties.class));
    }

    return canonicalConstructor;
  }

  private Optional<Object[]> parseQueryParamValues(Object rawValue) {
    if (rawValue == null) {
      return Optional.empty();
    }
    return Optional.ofNullable(unfold(rawValue).map(this::sanitize).toArray());
  }

  private Stream<?> unfold(Object rawValue) {
    if (rawValue instanceof Iterable<?>) {
      return StreamSupport.stream(((Iterable<?>) rawValue).spliterator(), false);
    }
    Class<?> rawValueClass = rawValue.getClass();
    if (rawValueClass.isArray()) {
      return Arrays.stream((Object[]) rawValue);
    }
    return Stream.of(rawValue);
  }

  private Object sanitize(Object value) {
    Class<?> valueClass = value.getClass();
    boolean isAuthorized =
        String.class.isAssignableFrom(valueClass) || Number.class.isAssignableFrom(valueClass);
    if (isAuthorized) {
      return value;
    }
    throw new IllegalArgumentException("Value %s has not an allowed type".formatted(value));
  }
}

And its tests:

import static org.assertj.core.api.Assertions.assertThat;
import static org.springframework.hateoas.server.mvc.WebMvcLinkBuilder.methodOn;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

import com.fasterxml.jackson.annotation.JsonProperty;
import java.beans.ConstructorProperties;
import javax.inject.Inject;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.hateoas.Link;
import org.springframework.hateoas.server.mvc.WebMvcLinkBuilderFactory;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.context.WebApplicationContext;

/** @author Réda Housni Alaoui */
@SpringBootTest
class ModelAttributeRecordUriContributorTest {

  @Inject private WebMvcLinkBuilderFactory webMvcLinkBuilderFactory;

  @Inject private WebApplicationContext context;

  private MockMvc mockMvc;

  @BeforeEach
  public void before() {
    this.mockMvc = MockMvcBuilders.webAppContextSetup(context).build();
  }

  @Test
  @DisplayName("Make request")
  void test1() throws Exception {
    mockMvc
        .perform(
            get("/ModelAttributeRecordUriContributorTest")
                .queryParam("first_param", "1")
                .queryParam("second_param", "2"))
        .andExpect(status().isOk())
        .andExpect(jsonPath("$.firstParam").value("1"))
        .andExpect(jsonPath("$.secondParam").value("2"));
  }

  @Test
  @DisplayName("Create link")
  void test2() {
    assertThat(
            webMvcLinkBuilderFactory
                .linkTo(methodOn(MyController.class).echo(new Params("1", "2")))
                .toUri())
        .hasToString("http://localhost/ModelAttributeRecordUriContributorTest?first_param=1&second_param=2");
  }

  @Test
  @DisplayName("Create link with null argument")
  void test3() {
    assertThat(
            webMvcLinkBuilderFactory
                .linkTo(methodOn(MyController.class).echo(new Params("1", (String) null)))
                .toUri())
        .hasToString("http://localhost/ModelAttributeRecordUriContributorTest?first_param=1");
  }

  @Test
  @DisplayName("URI template")
  void test4() {
    Link link =
        webMvcLinkBuilderFactory
            .linkTo(methodOn(MyController.class).echo(new Params(null, null)))
            .withRel("foo");
    assertThat(link.getHref())
        .isEqualTo("http://localhost/ModelAttributeRecordUriContributorTest{?first_param,second_param}");
  }

  @RequestMapping("/ModelAttributeRecordUriContributorTest")
  @Controller
  static class MyController {

    @GetMapping
    public ResponseEntity<?> echo(Params params) {
      return ResponseEntity.ok(params);
    }
  }

  private record Params(String firstParam, String secondParam) {

    @ConstructorProperties({"first_param", "second_param"})
    Params {}

    @JsonProperty
    public String firstParam() {
      return firstParam;
    }

    @JsonProperty
    public String secondParam() {
      return secondParam;
    }
  }
}

Please note that public TemplateVariables enhance( TemplateVariables templateVariables, UriComponents uriComponents, MethodParameter parameter) is coming from https://github.com/spring-projects/spring-hateoas/pull/1312/files#diff-98f07313975ba6946727f52d493a8d791bd49c640b95ee0869f32f42163da592R65 (a long standing PR without any answer :cry: ) .

If you want, I can make a pull request for this if https://github.com/spring-projects/spring-hateoas/pull/1312 or an alternative is merged.

reda-alaoui avatar Aug 29 '22 07:08 reda-alaoui

I can also make a PR without waiting for #1312 . It won't have template variable support.

reda-alaoui avatar Aug 29 '22 07:08 reda-alaoui

Well, Seems like a big problem, It bothers me now too

dingjiefeng avatar Nov 07 '22 16:11 dingjiefeng