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

linkTo_methodOn not working on GET requested object parameter #496

Open
khong07 opened this issue Sep 22, 2016 · 14 comments
Open

linkTo_methodOn not working on GET requested object parameter #496

khong07 opened this issue Sep 22, 2016 · 14 comments

Comments

@khong07
Copy link

khong07 commented Sep 22, 2016

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?

@odrotbohm
Copy link
Member

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.

@khong07
Copy link
Author

khong07 commented Sep 22, 2016

@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)

@otrosien
Copy link

@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.

@khong07
Copy link
Author

khong07 commented Jan 4, 2017

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

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

@jrauschenbusch
Copy link

jrauschenbusch commented Feb 18, 2018

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
Copy link

jrauschenbusch commented Feb 20, 2018

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
Copy link

jrauschenbusch commented Mar 16, 2018

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());

@tobspeed
Copy link

tobspeed commented Aug 8, 2018

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

@bob983
Copy link

bob983 commented Nov 14, 2018

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 :-)

@evainga
Copy link

evainga commented Jul 19, 2021

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

@odrotbohm
Copy link
Member

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.

@reda-alaoui
Copy link
Contributor

reda-alaoui commented Aug 29, 2022

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 😢 ) .

If you want, I can make a pull request for this if #1312 or an alternative is merged.

@reda-alaoui
Copy link
Contributor

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

@dingjiefeng
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants