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

closes #16 Modernize Spring apps structure #25

Merged

Conversation

mszarlinski
Copy link
Collaborator

This PR is based on @dsyer's spring-projects/spring-petclinic#200 and includes the following improvements:

  • Removed thin service layer
  • Introduced Lombok

@mszarlinski
Copy link
Collaborator Author

@arey please review

@arey
Copy link
Member

arey commented Dec 7, 2016

  • The business service layer has been removed. But I don’t see where are managed the database transactions? In the "Modularize and migrate to aggregate-oriented domain" commit, Dave moves @Transactional(readOnly = true) into the Repository interfaces (ie. OwnerRepository).
  • Do we still have to maintain all sub-packages? I mean: application, boundary.web, domain.model, support.jpa… In it’s commit, Dave moves all classes in a flat package (ie. org.springframework.samples.petclinic.owner). MVC, Repository and Domain classes are mixed. I’m not familiar with this new kind of organisation (comes from DDD ?) but he does.
  • PetValidator have been removed. I gave a check and I realized that server control has been removed in the Angular version. You may save a pet with an empty name despite the @SiZe(min = 1) String name on PetRequest inner class. You may do a test :
curl -H "Content-Type: application/json" -X PUT -d '{"id":1,"name":"","birthDate":"2010-09-07","typeId":"1"}' http://localhost:8080/api/customer/owners/1/pets/1

More over, duplicate names are not handled. See removed method: Owner::getPet(String name, boolean ignoreNew)
We may enable the server validation and returns an error message. May I create an issue in the angular fork?

  • I’ve noticed you've switched from Repository to JpaRepository. Why not.

@mszarlinski
Copy link
Collaborator Author

  • The business service layer has been removed. But I don’t see where are managed the database transactions? In the "Modularize and migrate to aggregate-oriented domain" commit, Dave moves @Transactional(readOnly = true) into the Repository interfaces (ie. OwnerRepository).

  • Do we still have to maintain all sub-packages? I mean: application, boundary.web, domain.model, support.jpa… In it’s commit, Dave moves all classes in a flat package (ie. org.springframework.samples.petclinic.owner). MVC, Repository and Domain classes are mixed. I’m not familiar with this new kind of organisation (comes from DDD ?) but he does.

    • I agree that we can flatten the package structure in this PR.
  • PetValidator have been removed. I gave a check and I realized that server control has been removed in the Angular version. You may save a pet with an empty name despite the @Size(min = 1) String name on PetRequest inner class. You may do a test :
    curl -H "Content-Type: application/json" -X PUT -d '{"id":1,"name":"","birthDate":"2010-09-07","typeId":"1"}' http://localhost:8080/api/customer/owners/1/pets/1
    More over, duplicate names are not handled. See removed method: Owner::getPet(String name, boolean ignoreNew) We may enable the server validation and returns an error message. May I create an issue in the angular fork?

    • Yes of course.
  • I’ve noticed you've switched from Repository to JpaRepository. Why not.

@mszarlinski mszarlinski merged commit d984ab4 into spring-petclinic:master Dec 12, 2016
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.

None yet

2 participants