pytanie i ocena kodu - REST (CRUD)

Cześć,

Mam do Was pytanie, dostałem zadanie poniżej
" Create a new REST API service with a new endpoint:

• Create /customer endpoint
• this is a Crud endpoint
  Database /Storage engine:
• choose a storage
• install the storage engine on local computer
• create datastructure for customer
• implement CRUD-functionality for customer
• Use Postman to test new endpoint

Zrobiłem wszystko jak powyżej, plus dodałem trochę estetyki i wszystko podpiąłem pod widoki.
Czy może ktoś ocenić czy jest zrobione zgodnie z instrukcją czy powinno być to zrobione na sucho tylko postman i CRUD przez URL

https://github.com/tomek1221/simpleCRUD

To co zrobiłeś nie ma nic wspólnego z zadaniem które otrzymałeś.
Pominę już to że zrobiłeś MVC zamiast CRUD'a restowego.

Tylko jak już zaczniesz pisać od nowa zwróć uwagę na parę rzeczy:

1. Nie pchamy w kontrolerze entity tylko DTO.
2. Nie znasz nic innego niż POST i GET ? Tj. np.

@RequestMapping(value = "/{id}/delete", method = RequestMethod.GET) // TO JEST FATALNE
@RequestMapping(value = "/{id}", method = RequestMethod.DELETE)
@DeleteMapping( "/{id}") // Jeszcze krócej

Istnieją też takie słówka jak Patch i Put poczytaj co do czego.
3. ResponseEntity zamiast ResponseBody
4. IMO jak to ma być pokazowy kod warto dodać dokumentację(swagger)
5. Obowiązkowo przynajmniej unit testy, jak dasz radę to integracyjne też.

Czyli żeby to napisać powinienem stworzyć Controller, który będzie miał lepiej rozpisane te metody czyli użyje też DELETE itd. i pominąć całkowicie widoki?

Chciałem po prostu to zrobić trochę ponad i żeby przypominało w jakimś stopniu już fragment strony tak dla treningu, dlatego też te widoki...

Masz może jakieś materiały solidne bez błędów w tym temacie?

1. Tak.
2. Zależy czy preferujesz książki czy online. Jedyne książki jakie miałem w rękach to REST. Najlepsze praktyki i wzorce w języku Java, i taką kobyłę REST API Design Rulebook, nie mam odniesienia i te książki czytałem, już mając podstawową wiedzę na ten temat, ale IMO pierwsza pozycja to takie szybkie wprowadzenie w tematykę i mniej więcej jak to powinno wyglądać tylko że w JAX-RS ;p. A druga to straszna kobyła do której podchodzę po raz 10, ale tam jest sama teoria.

Najbardziej polecam darmowe materiały z tej strony http://www.baeldung.com/rest-with-spring-series/

Ps. Jak korzystasz z H2 to weź wstaw skrypt inicjalizujące (jak resource wstawisz plik import.sql to automatycznie zaciągnie) dane a nie ładujesz je w main.

Jak chcesz szybko postawić projekt to obczaj sobie JHipster.

A co do błędów to pkt 1 i 3 z wcześniejszej odpowiedz, to nie jest świętość a raczej praktyka z którą najczęściej się spotkałem i dostrzegam jej zalety ;). Tak samo miejsce w którm np transformujesz entity na DTO. Niektórzy mówią, że w powinno się w serwisie kolejni, że w kontrolerze a jeszcze inni mają dodatkową warstwę biznes serwisów, i tam dopiero jest to zamieniane. Także raczej po prostu czytaj z wielu źródeł bo nie ma jednej prawdy :)

Schadoow napisał(a):

1. Nie pchamy w kontrolerze entity tylko DTO.

Nie robimy nonsensownego kodu jak nie trzeba. To jest CRUD. CRUD jest głupi. Ja bym tak zostawił.
(a w normalnym projekcie nie robiłbym CRUDa i wtedy problem by się rozwiązał)

Dodatkowo - tu takie Entity wychodzi ze standardowego DAO Springowego.
Właściwie już nic gorszego nie może się stać.
Controller i tak zwraca "jsony" (więc to jest nasze DTO).

Ok, zrobiłem to od nowa i wygląd to tak:

Customer.java

import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;

@Entity
public class Customer {
    @Id
    @GeneratedValue(strategy=GenerationType.AUTO)
    private Long id;
    private String name;
    private String address;
    private String countryCode;

	public String getCountryCode() {
		return countryCode;
	}

	public void setCountryCode(String countryCode) {
		this.countryCode = countryCode;
	}

	public Long getId() {
		return id;
	}

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

	public String getName() {
		return name;
	}

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

	public String getAddress() {
		return address;
	}

	public void setAddress(String address) {
		this.address = address;
	}
    
    
}

MainController.java

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.web.bind.annotation.*;

import java.util.List;

@RestController
@RequestMapping(path="/customer") 
public class MainController {

	@Autowired
	private CustomerRepository customerRepository;

	@GetMapping("/all")
	public List<Customer> getAll() {
		return customerRepository.findAll();
	}

	@PostMapping("/create")
	public List<Customer> create(@RequestBody Customer customer) {
		customerRepository.save(customer);
		return customerRepository.findAll();
	}

	@DeleteMapping("/delete/{id}")
	public List<Customer> remove(@PathVariable long id) {
		customerRepository.delete(id);
		return customerRepository.findAll();
	}

	@PutMapping("/update/{id}")
	public List<Customer> updateBook(@RequestBody Customer customer, @PathVariable Long id) {
		Customer currentuser = customerRepository.findOne(id);
		currentuser.setName(customer.getName());
		currentuser.setAddress(customer.getAddress());
		currentuser.setCountryCode(customer.getCountryCode());
		return customerRepository.findAll();
	}
}

CustomerRepository.java

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

@Repository
public interface CustomerRepository extends JpaRepository<Customer, Long> {

}

aplication.properties

spring.jpa.hibernate.ddl-auto=update
spring.datasource.url=jdbc:mysql://localhost:3306/db_example?autoReconnect=true&useSSL=false
spring.datasource.username=hbcustomer
spring.datasource.password=hbcustomer

Prośba do Was o ocenę na podstawie polecenia z pierwszego postu, dzięki za odpowiedzi :)

Moim zdaniem to trochę bez sensu teraz bo ten cały kontroler jest psu na budę potrzebny w takim projekcie. Równie dobrze mógłbyś sobie zrobic w tym repository

    @RestResource(
        rel = "findByCośtam",
        path = "findByCośtam"
    )

skoro i tak na pałe wystawiasz bazę danych przez resta.

A mógłbyś tak szerzej napisać, bo dopiero się tego uczę i nie rozumiem co to znaczy, że wystawiam bazę przez rest'a

I jakbyś to widział inaczej w takim razie? Jest w ogóle jakieś solidne źródło wiedzy w tym temacie, żeby tak koncepcyjnie zrozumieć po co niektóre rzeczy się robi?
Już jakiś czas to robię, czytam różne przykłady i czuje, że nic nie wiem...

Dwie wersje powyżej bazowały na przykładach z net'a więc już nie wiem gdzie szukać czegoś sprawdzonego.

Chodzi o to że ten przykład jest oderwany od rzeczywistości i generalnie bezużyteczny, a do tego można to zrobić trochę prościej, pomijając zbędny bojlerplejt. I tak przynajmniej nie zrobiłeś jeszcze warstwy "serwisów" które robiłyby tylko return respository.cośtam(), bo tak to zwykle pokazują w tutorialach. I potem ludzie zachodzą w głowę po co komu to wszystko :)

Napisz aplikacje która COŚ robi. Cokolwiek, ale niech to będzie coś.
A co do wystawiania bazy restem to przecież to właśnie robisz -> wyciągasz obiekty z bazy i wypychasz je z aplikacji restem w postaci jsona. Jest to generalnie bardzo zła praktyka, powodująca sporo problemów i błędów w "prawdziwym kodzie". Tylko że ludzie tak sie właśnie uczą, na głupich przykładach, i potem tak próbują robić.

@Shalom: ale jakie "coś" proponujesz? bo zaraz ktoś wyjedzie, żeby napisać cruda jak to zwykle bywa i wyjdzie to samo...

Oj nie wiem, niech się coś pod spodem dzieje. Np. zrób aplikacje gdzie podajesz imie i nazwisko a ona pobiera dane o danej osobie z różnych źródeł (taki mini stalker toolkit :P). Niby nadal crud, ot cośtam sie wpisuje do bazy danych, ale jednak ma jakąś logikę.

System do zarządzania wypożyczalnią <czegoś>?

Czyli warstwa serwisów jest po to żeby nie używać bezpośrednio bazy danych? Np.customerRepository.findAll(); ?

Podbije jeszcze raz o temat materiałów, coś co będzie mówiło jak się buduję większą aplikację w oparciu o ww frameworki, jakieś zasady, dobre praktyki. Będę bardzo wdzięczny.

@tomek1221 nie, warstwa modelu jest po to żeby gdzieś umieścić to co aplikacja robi. Aplikacja która tylko pisze i czyta z bazy to żadna aplikacja i równie dobrze można by ja zastąpić jakimś excelem :)

Zdaję sobie z tego sprawę, takie zadanie dostałem w ramach ćwiczeń i chciałbym je wykonać jak najbardziej poprawnie.

@Shalom: mam pytanie bo wiem, że jesteś ekspertem, czy wzięcie jakiejś istniejącej aplikacji i zrobienie jej "kopii" jest dobrym pomysłem na przećwiczenie skilla, jeśli sam nie mogę czegoś dobrego wymyśleć? przykładowo jakiś sklep czy coś i odwzorować go w miarę możliwości

Poza tymi co wyżej uwagami to na cholerę ci tam te słówka w tym uri ? Zmień to na:

    @GetMapping("/")
    @PostMapping("/")
    @DeleteMapping("/{id}")
    @PutMapping("/{id}")

Całą architekturę restów wyobraź sobie jako drzewo zasobów, czasowniki HTTP służą ci do modyfikacji zasobów. Jeśli nie musisz nie używaj czasowników w URI. (Wiadomo na akcje ofc ale nie do crudu...)

I jeszcze jedno chcąc być bardzo poprawnym IMO do edycji należało by użyc PATCH i przesyłać tylko zmodyfikowane pola.

Ok czyli co mam jeszcze zmienić żeby to było jak najbardziej poprawne w stosunku do treści zadania, które otrzymałem? zmienić te uri, zmienić edycje na PATCH, coś jeszcze?

Ilu programistów trzeba, aby napisać CRUDa ? - Nieskończenie wielu :D

Po pierwsze nie MainController a CustomersController.
Po drugie endpoint /customers na kontrolerze. (liczba mnoga)
Po trzecie żadne "all" w nazwie zasobu. Co oznacza GET /customer/all ? Pobierz wszystkie klienta? To nie trzyma się kupy.
Po czwarte preferuj wstrzykiwanie przez konstruktor zamiast pól. W tym przykładzie to bez znaczenia, ale ogólnie to lepsza praktyka.
Po piąte adnotacje, które wkleił Shalom to tzw. Spring Data Rest. Czyli narzędzie, które od razu z repozytorium robi resty bez tych wszystkich klas - przelotek.

Powinieneś mieć następujące endpointy:

Pobieranie wszystkich klientów - GET /customers
Dodawanie klienta POST /customers
Pobranie klienta GET /customers/{id}
Zamiana istniejącego klienta PUT /cystomers/{id}
Modyfikacja klienta PATCH /cystomers/{id}
Usunięcie klienta DELETE /customers/{id}

Zauważ, że w adresach nie ma czasowników, tylko drzewko rzeczowników / obiektów / zasobów.

@nie100sowny: Polecam zapoznać się: https://tools.ietf.org/html/rfc5789

HTTP PUT method only allows a complete replacement of a document.

Faktycznie, źle się wyraziłem. PUT podmiana na nowy, PATCH modyfikacja.

Dzięki Wam baaardzo za pomoc, już zaczęło mi się to wszystko trochę rozjaśniać :D

Teraz muszę dodać jeszcze kolejną bazę danych na produkty i zrobić CRUD-a na endpoint /products - spoko, rozumiem, że zrobię to na wzór tego co już powstało
aleeee ostatnia część zadania to:

"Create an orchestration service
Connect customer information and product information in the orchestration service.
New endpoint, get customer by id, returns:
One customer
List of products related to the customer"

I jakbyście mogli mi poradzić, jak za to się zabrać? Ja widzę to tak, dodaje do Customer, pole List<Products>, i daje na to adnotacje @ManyToMany tym samym tworząc relacje? Ale jakby miał wyglądąc "orchestration service"?

Pozdro i jeszcze raz dzięki za odpowiedzi.

Wiem, że w tym przypadku jest to mega rozdrobnienie i można skwitować to memem "więcej warstw" aczkolwiek.
Ja to rozumiem w ten sposób, że masz dwa serwisy jeden, dla produktów drugi dla klientów i teraz musisz to jakoś spiąc ze sobą ze względu na logikę biznesową więc IMO powinieneś stworzyć fasadę na te dwa serwisy tworząc nowy biznes-serwis.

Service orchestration

Tworzenie powiązań w encji jest spoko jeśli tworzy to spójny model, bo w pewnym momencie rozwoju aplikacji, dostaniesz kopa w tyłek jeśli chodzi o wydajność bo ORM będzie zaciągał cały graf, (tak tak wiem lazy loading i entity graphs) aczkolwiek nie jest wskazane wiązać zbyt dużej liczby modeli.

Dlatego DTO jest tak przydatne bo to jest obiekt tylko do reprezentacji( w którym możesz sobie zagregowac inne entity) to co dzieje się na niższych warstawach jest zupełnie niewidoczne z punktu widzenia użytkownika końcowego.

Muszę to przełożyć co napisałeś na moje toporne myślenie, w tym samym projekcie gdzie mam to co wcześniej zrobiłem, mam stworzyć jeszcze jeden serwis, który by łączył ze sobą Klientów i Produkty. OK, ale jak? Chyba musi być dodatkowo tabela, która będzie miała np ID produktu i ID klienta i np ten nowy serwis będzie miał endpoint /getCustomerById i wtedy wyświetli wszystkie produkty przypisane do klienta o danym ID?

Bo nie za bardzo rozumiem jak w inny sposób to można połączyć?

A np. jakbym dodał do Customer.java List<Product> listOfProducts z adnotacją @ManyToMany, to Hibernate utworzy mi automatycznie nową tabelę?

@tomek1221: Zależy co chcesz uzyskac nie musisz tego mappowac na poziomie encji. Tj musisz miec coś co łączy te wpisy. Przykład tylko dla zobrazowania jeśli chcesz zrobic kolejną warstwę bez łączenia entity.
(Ofc ci sie to nie skompiluje bo błędy składniowe i nie ma połowy rzeczy ale to tylko idea. )

@Entity
@Table(name = "A")
class A {
    @Basic
    @Column(name = "NAME")
    @Id
    private String name;

    @Basic
    @Column(name = "NICK")
    private String nick;
}

@Entity
@Table(name = "B")
class B {
    @Basic
    @Column(name = "ID")
    @Id
    private String id;

    @Basic
    @Column(name = "X")
    private String x;

    @Basic
    @Column(name = "NICK")
    private String nick;
}
public interface BRepository extends JpaRepository<B, String> {
    @Query("SELECT b FROM B b where b.nick = :nick") 
    Optional<List<B>> findBByNick(@Param("nick") String nick);
}

@Service("AService")
public AService{
   private ARepository aRepository;
   Optional<String> getNickByName(String name){
       return this.aRepository.findById(name)
   }
}

@Service("BService")
public BService{
   private BRepository bRepository;
   Optional<List<B>> getBByNick(String nick){
       return this.bRepository.findXByNick(nick)
   }
}

@Service("MojaZajebistaKolejnaWarstwa")
public MojaZajebistaKolejnaWarstwa{
    BService bService;
    AService aService;
    List<String> getListBByName(Name){
       Optional<String> nick = aService.getNickByName(name);
       if(nick.isPresent()){
        Optional<List<B>> xxxx = bService.getListX(nick.get())
        //cos tam cos tam cos tam .....
       }
    }
}

I teraz np potrzebujesz danych do widoku w twoim przypadku kontrolera, więc tworzysz sobie obiekt ABDTO w którym masz interesujące cię pola i tworzysz ten obiekt na podstawie informacji z A i B i go wysyłasz w zwrotce.

@Shadoow: Kurde sorry ale nie ogarniam tego co napisałeś, mógłbyś to przełożyć z A i B na Product i Customer? Bardzo będę wdzięczny, bo próbuje już sporo czasu coś z tym zrobić ale no nie idzie za nic...

Spoko nie jestem dobrym tłumaczem :p wieczorem jak ktoś inny ci nie wytłumaczy to przełożę. Wstaw wgl CAŁE zadanie bo mam wrażenie, że nie rozumiem co autor zadania chciał osiągnąć.

tomek1221 napisał(a):

@Shadoow: Kurde sorry ale nie ogarniam tego co napisałeś, mógłbyś to przełożyć z A i B na Product i Customer? Bardzo będę wdzięczny, bo próbuje już sporo czasu coś z tym zrobić ale no nie idzie za nic...

Chodzi o to, że masz stworzyć jeszcze jedną klasę, która będzie miała zależności do ProductService i CustomerService. W tej klasie masz sobie napisać logikę, która "obrobi" dane z obu serwisów jak sobie życzysz, stworzy jakąś listę DTO, która zostanie wypchnięta do kontrollera i restem w świat.

Ogólnie całe zadanie jakby składa się z trzech mniejszych, które opisałem wcześniej czyli:

1 - stowrzyć REST API z endpointem na /customer, zaimplementować do tego CRUD'a, i podpiąć to pod bazę danych
2 - to samo co powyżej tylko, że z endpointem na /products i jeżeli dobrze to rozumiem to kolejną bazę danych pod Products (Database/Storage engine
Create data structure for products)

3 -
Create an orchestration service
Connect customer information and product information in the orchestration service.
New endpoint, get customer by id, returns:
One customer
List of products related to the customer

Jeszcze raz dzięki za zaangażowanie :)

@artur52: Tylko problem w tym, że ja w swoim kodzie nie mam w ogóle serwisów, mam CustomerRepository i ProductRepository, mogę ich do tego użyć czy powinienem stworzyć warstwę serwisów?