Skip to content

Path variable resolved as empty string #1728

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

Open
ecattez opened this issue Dec 9, 2021 · 6 comments
Open

Path variable resolved as empty string #1728

ecattez opened this issue Dec 9, 2021 · 6 comments

Comments

@ecattez
Copy link

ecattez commented Dec 9, 2021

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

ecattez commented Dec 10, 2021

I think the issue is because of his line:

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

3d7458b#diff-09a17eb7868f85c16a8ee6712c5fdbac7c32c58c83226b78cbc0b980bf8d798c

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

@oliverhenlich
Copy link

oliverhenlich commented Nov 23, 2023

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

oliverhenlich commented Dec 1, 2023

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 added a commit to Unimarket/spring-hateoas that referenced this issue Dec 1, 2023
@odrotbohm
Copy link
Member

odrotbohm commented Dec 1, 2023

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.

@oliverhenlich
Copy link

oliverhenlich commented Dec 5, 2023

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.

@odrotbohm
Copy link
Member

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.

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

No branches or pull requests

3 participants