generator-jhipster icon indicating copy to clipboard operation
generator-jhipster copied to clipboard

make id param real optional in PUT/PATCH rest api

Open iconben opened this issue 3 years ago • 1 comments

Currently, the id parameter in a POST/PUT API (e.g. /api/posts/:id) is claimed not required as in @PathVariable(required = false), but is not really optional, ignoring this parameter will produce a null pointer error.

Code generated by current template:

    @PutMapping("/posts/{id}")
    public ResponseEntity<Post> updatePost(@PathVariable(value = "id", required = false) final Long id, @Valid @RequestBody Post post)
        throws URISyntaxException {
        log.debug("REST request to update Post : {}, {}", id, post);
        if (post.getId() == null) {
            throw new BadRequestAlertException("Invalid id", ENTITY_NAME, "idnull");
        }
        if (!Objects.equals(id, post.getId())) {
            throw new BadRequestAlertException("Invalid ID", ENTITY_NAME, "idinvalid");
        }

        if (!postRepository.existsById(id)) {
            throw new BadRequestAlertException("Entity not found", ENTITY_NAME, "idnotfound");
        }

        Post result = postService.save(post);
        return ResponseEntity
            .ok()
            .headers(HeaderUtil.createEntityUpdateAlert(applicationName, true, ENTITY_NAME, post.getId().toString()))
            .body(result);
    }

This merge request is to allow the id parameter to be really optional. This also benefits those legacy API consumers that do not pass an id param according to ancient versions.

Code generated after the change:

    @PutMapping("/posts/{id}")
    public ResponseEntity<Post> updatePost(@PathVariable(value = "id", required = false) final Long id, @Valid @RequestBody Post post)
        throws URISyntaxException {
        log.debug("REST request to update Post : {}, {}", id, post);
        if (post.getId() == null) {
            throw new BadRequestAlertException("Invalid id", ENTITY_NAME, "idnull");
        }
        if (id != null && !Objects.equals(id, post.getId())) {
            throw new BadRequestAlertException("Invalid ID", ENTITY_NAME, "idinvalid");
        }

        if (!postRepository.existsById(post.getId())) {
            throw new BadRequestAlertException("Entity not found", ENTITY_NAME, "idnotfound");
        }

        Post result = postService.save(post);
        return ResponseEntity
            .ok()
            .headers(HeaderUtil.createEntityUpdateAlert(applicationName, true, ENTITY_NAME, post.getId().toString()))
            .body(result);
    }

Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

iconben avatar Jul 08 '22 03:07 iconben

This api changed at v7 beta cycle, we are about to start v8. IMO we should fix the null pointer problem, but keep the api. Unless there are some compelling reason.

mshima avatar Jul 11 '22 18:07 mshima

The reason path variable id is nullable is to generate the correct error trough api, if we make sure the error is ok, we should set to required.

mshima avatar Aug 17 '22 11:08 mshima