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

Path variable resolved as empty string

Open ecattez opened this issue 3 years ago • 6 comments
trafficstars

Since version 1.4, path variables are resolved as empty strings instead of template variables in target field:

The link creation:

linkTo(methodOn(SelectColorResource.class)
                .selectColor(null, null))
                .withRel("select-color");

The SelectColorController

@PostMapping("/select-color)
    public RepresentationModel selectColor(
            @PathVariable ProductId productId,
            @Valid @RequestBody SelectColorRequest request) {
        ...
    }

Before 1.4:

{
"method":"POST",
"properties":[{"name":"reference","required":true,"type":"text"}],
"target":"http://localhost/products/{productId}/select-color"
}

Since 1.4:

{
"method":"POST",
"properties":[{"name":"reference","required":true,"type":"text"}],
"target":"http://localhost/products//select-color"
}

ecattez avatar Dec 09 '21 13:12 ecattez

I think the issue is because of his line:

String target = it.getLink().expand().getHref();

https://github.com/spring-projects/spring-hateoas/commit/3d7458b5f6912c6ccd553cda8345ca6c62826506#diff-09a17eb7868f85c16a8ee6712c5fdbac7c32c58c83226b78cbc0b980bf8d798c

It expands the link so if there is path variables, they're replaced by empty strings.

ecattez avatar Dec 10 '21 14:12 ecattez

Hi,

We are currently upgrading our spring boot application from spring boot 2.7.10 -> 3.1.5. This pulls in a newer version of spring-hateoas which we'd like to move too as well (1.3.5 -> 2.1.2). However a whole bunch of our unit tests are failing because of this .expand().

Just want to report that it also happens for @RequestParam params whose values are not specified at link creation time. Please see here for specifics https://stackoverflow.com/questions/77526427/target-url-placeholders-missing-after-upgrading-from-spring-hateoas-1-3-5-to-2-1

At this stage I'd just like to understand if this is an intentional change in behaviour, incorrect usage of spring hateoas by us or simply a regression.

CC: @odrotbohm

oliverhenlich avatar Nov 23 '23 05:11 oliverhenlich

Hi @odrotbohm

It would be great if you could have a look that the PR I just linked to this issue. I made a small change to the HalFormsWebMvcIntegrationTest test case to demonstrate our problem. Based on the latest state of 2.1.x.

Our basic usage pattern is to have a resource that clients can get a list of results from (PagedModel). And this model has some affordances added to it as normal (e.g. create a new item). Some of them have additional affordances (POST requests) that advertise to the client that there are other operations they can perform on the list. But they need to provide input to the request. What we do and expect exactly is described better in the linked stackoverflow post.

At this point we'd just appreciate some feedback on whether what we are doing and/or expecting is not valid in some way or whether this is simply a regression?

oliverhenlich avatar Dec 01 '23 02:12 oliverhenlich

The change you stumble above was introduced to align with the HAL Forms spec. Values for target must be URIs, not URI templates. The alternative to expanding would be to completely fail the rendering with a hint towards the invalid link creation. Does that sound better?

EDIT: That's even obvious from the change you linked to and the in turn linked issue. See also the discussion attached to the commit. I guess HAL FORMS expects all variable aspects of the transition to be in the form itself, and thus a potential template would have to be expanded on rendering the form. This doesn't seem to be too far fetched as HAL FORMS is very much inspired by HTML forms.

odrotbohm avatar Dec 01 '23 12:12 odrotbohm

Thanks for the reply @odrotbohm and for pointing out the discussion on that commit that I'd missed.

It does feel like failing rendering might be better than silently producing links containing empty strings (e.g. bla2//bla2).

As well as the problem described in my stackoverflow issue (target with query parameter missing the template variable), this change has thrown a spanner in the works in another area.

We have resources as follows

  1. /orders
  2. /orders/{orderId}
  3. /orders/{orderId}/items
  4. /orders/{orderId}/items/{itemId}

For 3. our response looks like this:

{
  "_embedded": {
    "items": [
      {
        ...<snip>,
        "_links": {
          "self": {
            "href": "http://localhost/orders/b59e1721-69d0-4b5d-8fec-dc854adcbd1d/items/e25d9510-f947-429b-9610-15728bad2609",
            "name": "e25d9510-f947-429b-9610-15728bad2609"
          },
          "order": {
            "href": "http://localhost/orders/b59e1721-69d0-4b5d-8fec-dc854adcbd1d",
            "title": "O1",
            "name": "b59e1721-69d0-4b5d-8fec-dc854adcbd1d"
          }
        },
      }
    ]
  },
  "_links": {
    "self": {
      "href": "http://localhost/orders/b59e1721-69d0-4b5d-8fec-dc854adcbd1d/items"
    }
  },
  "page": {
    "size": 1,
    "totalElements": 1,
    "totalPages": 1,
    "number": 0
  },
  "_templates": {
    "default": {
      "method": "POST",
      "properties": [],
      "target": "http://localhost/orders/{orderId}/items/something"
    }
  }
}

You'll see we emit a template in the root of the response to perform some action at the level of the resource. Here we don't know what the specific order would be and hence the target used to include the {orderId} placeholder. Now however the target is rendered as http://localhost/orders//items/something and I don't know how to work around it (In the query parameter case a workaround is to move the required data into the body of the request instead).

Any thoughts would be much appreciated.

oliverhenlich avatar Dec 05 '23 03:12 oliverhenlich

For that particular resource, wouldn't the (already fully expanded) self link be the target anyway? In other words, you shouldn't even need to explicitly declare a target. An easy way to do this is:

Affordances.of(selfLink)
  .afford(…) // build up templates
  .toLink();

That'll use the selfLink as basis and target for all templates, unless you explictly configure a ….withTarget(…) on the affordance built up.

Regarding the optionality, I fear we're stuck with the definitions in the RFC and that defines "undefined" values to be skipped unconditionally. In other words, if we reject the expansion, we'd not comply with the specification anymore.

odrotbohm avatar Dec 05 '23 09:12 odrotbohm