Rest Api with generic crud operations

Question

I am learning how to create a rest api in which I've decided to use generic for crud operation. I am attaching the code which I have written so far. Please have a look. Thanks for the review in advance.

What I expect from code review:

1. Good part of the code (if any)
   
2. Code portion which needs improvement(or any alternate way)
   
3. Code which should not be there.
   
4. Any source from where I can learn how to create the architecture of web-app.

Brand Model

package com.test.model;
import java.io.Serializable;

import javax.persistence.Cacheable;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.Table;

import org.hibernate.annotations.Cache;
import org.hibernate.annotations.CacheConcurrencyStrategy;


@Entity
@Cacheable
@Cache(usage=CacheConcurrencyStrategy.READ_WRITE)
@Table(name = "brand_master")
public class BrandMaster extends BaseEntity implements Serializable {

    private static final long serialVersionUID = 4045918990469712465L;

    @Id
    @GeneratedValue(strategy = GenerationType.SEQUENCE)
    @Column(name = "pk_brandId")
    private Integer id;

    @Column(name = "brand_name", unique = true)
    private String name;

    public Integer getId() {
        return id;
    }

    public void setId(Integer id) {
        this.id = id;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

} 

BaseEntity:

package com.test.model;

import java.util.Date;

import javax.persistence.Column;
import javax.persistence.EntityListeners;
import javax.persistence.MappedSuperclass;

import org.springframework.data.annotation.CreatedBy;
import org.springframework.data.annotation.CreatedDate;
import org.springframework.data.annotation.LastModifiedBy;
import org.springframework.data.annotation.LastModifiedDate;
import org.springframework.data.jpa.domain.support.AuditingEntityListener;
import org.springframework.format.annotation.DateTimeFormat;

@MappedSuperclass
@EntityListeners(AuditingEntityListener.class)
public class BaseEntity {

    @CreatedDate
    @Column(name = "createdOn")
    @DateTimeFormat(pattern = "dd/MM/yyyy")
    protected Date createdOn;

    @CreatedBy
    @Column(name = "createdBy")
    protected UserMaster createdBy;

    @LastModifiedDate
    @Column(name = "updateOn")
    @DateTimeFormat(pattern = "dd/MM/yyyy")
    protected Date updatedOn;

    @LastModifiedBy
    @Column(name = "updatedBy")
    protected UserMaster updatedBy;

    public Date getCreatedOn() {
        return createdOn;
    }

    public void setCreatedOn(Date createdOn) {
        this.createdOn = createdOn;
    }

    public UserMaster getCreatedBy() {
        return createdBy;
    }

    public void setCreatedBy(UserMaster createdBy) {
        this.createdBy = createdBy;
    }

    public Date getUpdatedOn() {
        return updatedOn;
    }

    public void setUpdatedOn(Date updatedOn) {
        this.updatedOn = updatedOn;
    }

    public UserMaster getUpdatedBy() {
        return updatedBy;
    }

    public void setUpdatedBy(UserMaster updatedBy) {
        this.updatedBy = updatedBy;
    }
}

Controller

package com.test.controller;

import java.util.List;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

import com.test.dto.BrandDTO;
import com.test.service.BrandService;

@RestController
@RequestMapping(path = "**/Brand")
public class BrandController {

    private Logger log = LoggerFactory.getLogger(BrandController.class);

    @Autowired
    private BrandService BrandService;

    @GetMapping(path = "**/{id}")
    public ResponseEntity<BrandDTO> get(@PathVariable Integer id) {
        log.trace("Starting processing get request for id :" + id);

        BrandDTO Brand = BrandService.get(id);
        if (Brand != null) {
            return new ResponseEntity<BrandDTO>(Brand, HttpStatus.OK);
        }

        log.info("Entity having id not found, id : " + id);
        return new ResponseEntity<BrandDTO>(HttpStatus.NOT_FOUND);
    }

    @GetMapping
    public ResponseEntity<List<BrandDTO>> getAll(@RequestParam(value = "pageNum", required = false) Integer pageNum, @RequestParam(value = "size", required = false) Integer size) {
        log.trace("Starting processing getAll request!");

        List<BrandDTO> Brandes = BrandService.getAll(pageNum, size);
        if (Brandes != null && !Brandes.isEmpty()) {
            return new ResponseEntity<List<BrandDTO>>(Brandes, HttpStatus.OK);
        }

        log.info("No element found while hitting getAll");
        return new ResponseEntity<List<BrandDTO>>(HttpStatus.NO_CONTENT);
    }

    @PostMapping
    public ResponseEntity<BrandDTO> save(@RequestBody BrandDTO Brand) {
        log.trace("Starting processing Post request!");

        try {
            Brand = BrandService.saveOrUpdate(Brand);
        } catch (Exception err) {
            log.error("Error Occured while saving, Message : " + err.getMessage() + "; Cause :" + err.getCause());
            return new ResponseEntity<BrandDTO>(HttpStatus.INTERNAL_SERVER_ERROR);
        }
        return new ResponseEntity<BrandDTO>(Brand, HttpStatus.CREATED);
    }

    @PutMapping(path = "**/{id}")
    public ResponseEntity<BrandDTO> update(@PathVariable Integer id, @RequestBody BrandDTO Brand) {

        log.trace("Starting processing put for id :" + id);

        if (Brand != null && Brand.getId() == null) {
            log.trace("updating id in model for put request for id :" + id);
            Brand.setId(id);
        }

        if (Brand != null && Brand.getId().equals(id)) {
            log.trace("processing put request for id :" + id);

            try {
                Brand = BrandService.saveOrUpdate(Brand);
            } catch (Exception err) {
                log.error("Error Occured while saving, Message : " + err.getMessage() + "; Cause :" + err.getCause());
                return new ResponseEntity<BrandDTO>(HttpStatus.INTERNAL_SERVER_ERROR);
            }
            return new ResponseEntity<BrandDTO>(Brand, HttpStatus.OK);
        }

        log.info("Id mismatch for model (" + Brand.getId() + ") and request param :" + id);
        return new ResponseEntity<BrandDTO>(HttpStatus.NOT_MODIFIED);
    }

    @DeleteMapping(path = "**/{id}")
    public ResponseEntity<String> delete(@PathVariable Integer id) {
        log.trace("Starting  processing of delete rrequest for id :" + id);

        if (!BrandService.isExist(id)) {
            log.info("Entity not found while processing the delete request for id :" + id);
            return new ResponseEntity<>(HttpStatus.NOT_FOUND);
        }

        BrandService.delete(id);
        log.info("Entity deleted having id :" + id);
        return new ResponseEntity<>(HttpStatus.OK);
    }

}

Service:

package com.test.service;

import com.test.dto.BrandDTO;
import com.test.model.BrandMaster;

public interface BrandService extends GenericService<BrandMaster, Integer, BrandDTO> {

}

Service Impl:

package com.test.service.impl;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;

import com.test.dao.BrandDao;
import com.test.dto.BrandDTO;
import com.test.mapper.BrandMapper;
import com.test.model.BrandMaster;
import com.test.service.BrandService;

@Service("brandService")
public class BrandServiceImpl extends GenericServiceImpl<BrandMaster, Integer, BrandDTO> implements BrandService{

    @Autowired
    public BrandServiceImpl(BrandDao brandDao) {
        super(brandDao);
    }

    @Override
    public BrandMaster transformDTOToEntity(BrandDTO element) {
        return BrandMapper.INSTANCE.brandDTOToBrandMaster(element);
    }

    @Override
    public BrandDTO transformEntityToDTO(BrandMaster element) {
        return BrandMapper.INSTANCE.brandMasterToBrandDTO(element);
    }


}

Generic Service

package com.test.service;

import java.util.List;

public interface GenericService <E, I, D> {

    /**
     * E : Entity Class
     * I : type of Id element
     * D : DTO POJO
     */

    public D get(I id);
    public List<D> getAll(Integer pageNumber, Integer size);
    public D saveOrUpdate(D element);
    public void delete(I id);
    public boolean isExist(I id);
    public E transformDTOToEntity(D element);
    public D transformEntityToDTO(E element);
}

Generic Service Impl

package com.test.service.impl;

import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
import org.springframework.data.jpa.repository.JpaRepository;

import com.test.service.GenericService;

public abstract class GenericServiceImpl <E, I, D> implements GenericService<E, I, D>{

    protected JpaRepository<E, I> repository;

    private Logger log = LoggerFactory.getLogger(GenericServiceImpl.class);

    public GenericServiceImpl(JpaRepository<E, I> repository) {
        this.repository = repository;
    }

    @Override
    public D get(I id) {
        log.trace("<== Inside generic get method ==> ");

        Optional<E> element = repository.findById(id);
        if(element.isPresent()){
            log.trace("<== Got the response from dao and received an object ==>");
            return this.transformEntityToDTO(element.get());
        }

        log.info("<== No element is found for the passed id ==>");
        return null;
    }

    @Override
    public List<D> getAll(Integer pageNumber, Integer size) {
        log.trace("<== Inside getAll() of generic service ==>");


        pageNumber = pageNumber != null? pageNumber : 0;
        size = size != null? size : 10;

        Pageable page = PageRequest.of(pageNumber, size); 
        List<E> list = repository.findAll(page).getContent();

        log.trace("<== Got the response from the repository from getALL  ==>");
        return list.stream().map(entity -> transformEntityToDTO(entity)).collect(Collectors.toList());
    }

    @Override
    public D saveOrUpdate(D element) {
        log.trace("<== Inside sabeOrOpdate() of generic service ==>");

        return this.transformEntityToDTO(repository.save(transformDTOToEntity(element)));
    }

    @Override
    public void delete(I id) {
        log.trace("<== Inside delete() of generic service ==>");

        repository.deleteById(id);
    }

    @Override
    public boolean isExist(I id) {
        log.trace("<== Inside isExist() of generic service ==>");

        return repository.findById(id).isPresent();
    }

    @Override
    public E transformDTOToEntity(D element) {
        return null;
    }

    @Override
    public D transformEntityToDTO(E element) {
        return null;
    }


}

Brand DTO

package com.test.dto;

import java.io.Serializable;

public class BrandDTO implements Serializable{

    private static final long serialVersionUID = -8226355260297089645L;
    private Integer id;
    private String name;

    public Integer getId() {
        return id;
    }
    public void setId(Integer id) {
        this.id = id;
    }
    public String getName() {
        return name;
    }
    public void setName(String name) {
        this.name = name;
    }


}

Mapper

package com.test.mapper;

import org.mapstruct.Mapper;
import org.mapstruct.factory.Mappers;

import com.test.dto.BrandDTO;
import com.test.model.BrandMaster;

@Mapper
public interface BrandMapper {

    BrandMapper INSTANCE = Mappers.getMapper(BrandMapper.class);

    BrandDTO brandMasterToBrandDTO(BrandMaster brand);

    BrandMaster brandDTOToBrandMaster(BrandDTO brand);

}

Brand Dao

package com.test.dao;

import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

import com.test.model.BrandMaster;

@Repository
public interface BrandDao extends JpaRepository<BrandMaster, Integer>{

}

Answer 1

Your code is good, but some improvements could be made. Here are my suggestions:

1. Use lowercase for first letter of variable. Example: BrandService BrandService; rename to: BrandService brandService;

2. Use Lombok annotations like @Getter, @Setter, @Slf4j and various @*Constructors and so on. It will make code clean and readable.

3. Use ControllerAdvice for global error handling:

   @ControllerAdvice
   @Slf4j
   public class ExceptionHandlingAdvice {
       private final Clock clock = Clock.systemDefaultZone();
   
       @ExceptionHandler(Exception.class)
       public ResponseEntity<ErrorResponse> handleException(Exception ex) {
           return handleException(ex.getLocalizedMessage(), HttpStatus.INTERNAL_SERVER_ERROR);
       }
   
       private ResponseEntity<ErrorResponse> handleException(String message, HttpStatus httpStatus) {
           log.error("Error: " + message);
           ErrorResponse errorResponse = ErrorResponse.of(Instant.now(clock), message);
           return new ResponseEntity<>(errorResponse, httpStatus);
       }
   }
   

4. Use dedicated errors for specific cases.

5. It is a good approach to move @Transactional to the service layer. If you call two DAO-methods on the service level they use the same transaction.

6. Can't see any reason for a DTO to implement Serializable.

7. Use lowercase in URI paths, example: @RequestMapping(path = "**/Brand") should be **/brand

8. Consider to add content-negotiation (for respective HTTP-headers Accept and Content-Type) to your controller-mapping:

   @PostMapping(consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE)
   

9. In my opinion you use a too abstract path mapping:

• @RequestMapping(path = "**/Brand") could be @RequestMapping(path = "/api/brand")
• @GetMapping(path = "**/{id}") could be @GetMapping(path = "/{id}")

10. You could use @ResponseStatus to Set HTTP Status Code and return just object instead of wrapping into ResponseEntity

    @PostMapping
    @ResponseStatus(HttpStatus.CREATED)
    public BrandDTO save(@RequestBody BrandDTO Brand) {
        // omitted
        return brand;
    }