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

Support for HAL-FORMS value element #2039

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reda-alaoui
Copy link
Contributor

@reda-alaoui reda-alaoui commented Nov 7, 2023

Fixes #1717

Behaviour before this change

Let be:

  class MyController {
    @GetMapping
    public ResponseEntity<?> list() {
      Link selfLink = selfLink.andAffordance(afford(methodOn(MyController.class).create(null)));
      return ResponseEntity.ok(new RepresentationModel<>(selfLink));
    }
  
    @PostMapping
    public ResponseEntity<?> create(@RequestBody Payload payload) {
      return ResponseEntity.created().build();
    }
  }
  
  record Payload(String foo, String bar) {
    Payload {
     // 'foo' and 'bar' are mandatory
     // we verify this invariant in the constructor to automatically end up with an HTTP 400 bad request in case of thrown exception
     requireNonNull(foo);
     requireNonNull(bar);
   }
  }

Calling GET will return something like:

{
  "_templates": {
    "default": {
      "properties": [
        {
           "name": "foo"
         },
         {
           "name": "bar"
         }
      ]
    }
  }
}

We want to be able to assign a default value hello to attribute foo in the HAL-FORMS payload to obtain something like this:

{
  "_templates": {
    "default": {
      "properties": [
        {
           "name": "foo",
           "value": "hello"
         },
         {
           "name": "bar"
         }
      ]
    }
  }
}

Solution brought by this change

The implemented solution is heavily inspired from HalFormsOptionsFactory. A consumer can provide a value creator, taking a property metadata as input and returning a value of type String as output.

Considered alternatives

Retrieve the value directly from the payload instance

To do that, this kind of consumer code would be needed:

@GetMapping
public ResponseEntity<?> list() {
    Link selfLink = selfLink.andAffordance(afford(methodOn(MyController.class).create(new Payload("hello", null))));
    return ResponseEntity.ok(new RepresentationModel<>(selfLink));
}
  1. this will not work since bar is verified for non-nullity in Payload constructor.
  2. consumers will not either accept to relax the constructor invariant validation

On the 2nd point, some may argue that jakarta.validation.constraints.NotNull should be used instead of the in-house constructor validation. On that my opinion is as follow:

  1. @NotNull should be avoided when plain simple java code is able to enforce the same constraint
  2. IDEs will probably at least emit a warning when seeing a null value assigned to a @NotNull property, at worst will fail the compilation

Retrieve the value from a new annotation on the considered property

This would force the consumer to know the value before compilation.

@reda-alaoui reda-alaoui marked this pull request as ready for review November 7, 2023 14:15
@odrotbohm odrotbohm force-pushed the main branch 4 times, most recently from 5828e78 to e643c37 Compare November 16, 2023 10:46
@kalgon
Copy link

kalgon commented Sep 16, 2024

Regarding the first considered alternative (Retrieve the value directly from the payload instance), maybe using
jakarta.validation.constraints.NotNull with a validation group could be an acceptable trade-off that won't trigger the IDE?

public class Payload {

    // constructor, getters, setters

    @NotNull(groups = Submitted.class)
    private String foo;
    
    @NotNull(groups = Submitted.class)
    private String bar;
}

public class MyController {
 
    @GetMapping
    public ResponseEntity<?> list() {
        Link selfLink = selfLink.andAffordance(afford(methodOn(MyController.class).create(new Payload("hello", null))));
        return ResponseEntity.ok(new RepresentationModel<>(selfLink));
    }

    @PostMapping
    public ResponseEntity<?> create(@RequestBody @Validated(Submitted.class) Payload payload) {
        return ResponseEntity.created().build();
    }
}

@reda-alaoui
Copy link
Contributor Author

@kalgon , when I look at this, I see an annotation hell ^^

@reda-alaoui
Copy link
Contributor Author

@odrotbohm, could we have some feedback on this? I'd need the same thing for HalFormsOptions to be able to select the pre-selected option from the payload.

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

Successfully merging this pull request may close these issues.

HalForm property value not set
2 participants