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

HAL Forms properties should consider Jackson constructors #1274

Open
reda-alaoui opened this issue Apr 21, 2020 · 8 comments
Open

HAL Forms properties should consider Jackson constructors #1274

reda-alaoui opened this issue Apr 21, 2020 · 8 comments
Assignees
Labels
in: mediatypes Media type related functionality type: enhancement

Comments

@reda-alaoui
Copy link
Contributor

reda-alaoui commented Apr 21, 2020

Hello,

Let's consider the following controller:

@RequestMapping("/foos")
public class FooController {

  @GetMapping
  public ResponseEntity<?> list() {
    return ResponseEntity.ok(
          new CollectionModel<>(
              Collections.emptyList(),
              selfLink.andAffordance(
                  afford(
                      methodOn(FooController.class).create(null)))));
  }

  @PostMapping
  public ResponseEntity<?> create(@RequestBody CreateCommand command) {
    //...
  }

  private static class CreateCommand {
    private final String name;
    
    @JsonCreator
    CreateCommand(@JsonProperty("name") String name){
      this.name = name;
    }
  }
}

FooController#list produces:

{
  "_links": {
    "self": {
      "href": "http://localhost:8080/foos"
    }
  },
  "_templates": {
    "default": {
      "method": "post",
      "properties": []
    }
  }
}

As you can see, property name is missing.

@gregturn gregturn added in: mediatypes Media type related functionality type: enhancement labels Apr 21, 2020
@gregturn gregturn added this to the 1.1.0.RELEASE milestone Apr 21, 2020
@gregturn gregturn self-assigned this Apr 21, 2020
@gregturn
Copy link
Contributor

Intriguing.

I crafted a test case matching your scenario, and it actually showed the attribute you said was missing. But I tracked it down to the getter method.

So this gives me a good arena to work on.

@reda-alaoui
Copy link
Contributor Author

What do you mean by I tracked it down to the getter method?
There is no getter in the scenario.

I forgot to say that jackson autodetection is disabled in my environment (#1147):

objectMapper
        .disable(AUTO_DETECT_CREATORS)
        .disable(AUTO_DETECT_FIELDS)
        .disable(AUTO_DETECT_GETTERS)
        .disable(AUTO_DETECT_IS_GETTERS)
        .disable(AUTO_DETECT_SETTERS)
        .disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);

And I am using version 1.0.4.RELEASE

@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Apr 21, 2020

And please note that adding getter getName for name makes the property appear but with readonly : true.

@gregturn
Copy link
Contributor

What I meant is that as soon as Spring HATEOAS can spot a getName(), it will transform that into a property of called "name", and even evaluate Jackson annotations related to the getter. It will also uses the existence of setName() to glean the value of readyOnly.

By dropping that, I am working on a way to read the constructor method with JsonCreator annotation, and find all the JsonProperty -related attributes.

The one thing to figure out is if there are both (a getName() AND a JsonCreator constructor call), which one takes precedence. My assumption would be the getter.

Also, if this works @odrotbohm and I can assess whether or not to backport it to 1.0.

@gregturn
Copy link
Contributor

gregturn commented Apr 21, 2020

I can see test cases encompassing:

private static class CreateCommand {

    String name;

    @JsonCreator
    CreateCommand(@JsonProperty("otherName") String name) {
        this.name = name;
    }
}

The resulting JSON should use otherName. However, readOnly is a bit fuzzy. There is no setter, but you CAN populate via the constructor.

@Data
private static class CreateCommand {

    String name;

    @JsonCreator
    CreateCommand(@JsonProperty("otherName") String name) {
        this.name = name;
    }
}

Should this one find name, but then let otherName supercede it? Or should both properties stand given there is a getter and setter for name?

@reda-alaoui
Copy link
Contributor Author

I would say both properties stand.
Because, IMO, we can’t be sure that name attribute is initialised with otherName value inside the constructor.

@reda-alaoui
Copy link
Contributor Author

Although, maybe jackson just ignores any setter when it finds a JsonCreator?

gregturn added a commit that referenced this issue Apr 22, 2020
When a value object has a constructor with Jackson @JsonCreator settings, use it to gather properties for HAL-FORMS.
@gregturn gregturn modified the milestones: 1.1.0.RELEASE, 1.2.0-M1 Apr 29, 2020
@gregturn
Copy link
Contributor

@odrotbohm and I discussed this in a bit more detail, and realized there is a whole story connected to this that needs to be evaluated. Thus, we've moved this into 1.2.

Essentially, it's possible to create an immutable type where you have no setters. Instead, everything is built upon either constructor calls or "@With" or whatever, and the existing code would PROBABLY misread the "readOnly" nature of those types.

We need to expand the existing set of unit tests to cover such scenarios, and then potentially entertain a "hint" annotation that could be applied to properties, constructors, and even getters/setters. That way, Spring HATEOAS can try to guess as best as it can, but in the corner cases where it can't figure it out, grant users ability to mark up their code to help HAL-FORMS render things as desired.

@odrotbohm odrotbohm modified the milestones: 1.2 M1, 1.2 RC1 Aug 11, 2020
@gregturn gregturn modified the milestones: 1.2 RC1, 1.2 GA Sep 15, 2020
@odrotbohm odrotbohm modified the milestones: 1.2 GA, 1.3 M1 Oct 27, 2020
@odrotbohm odrotbohm modified the milestones: 1.3 M1, 1.3 M2 Jan 13, 2021
@odrotbohm odrotbohm modified the milestones: 1.3 M2, 1.3 RC1 Feb 16, 2021
@odrotbohm odrotbohm modified the milestones: 1.3 M3, 1.4 M1 Mar 15, 2021
@odrotbohm odrotbohm modified the milestones: 1.4 M1, 1.4 M2 Jul 14, 2021
@odrotbohm odrotbohm modified the milestones: 1.4 M2, 1.4 M3 Aug 12, 2021
@odrotbohm odrotbohm modified the milestones: 1.4 M3, 1.4 candidates Sep 16, 2021
@odrotbohm odrotbohm removed this from the 1.4 candidates milestone Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: mediatypes Media type related functionality type: enhancement
Projects
None yet
Development

No branches or pull requests

3 participants