Modularize and migrate to aggregate-oriented domain #200

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@dsyer
Member

dsyer commented Nov 11, 2016

Vet, Owner, Visit. The Visit "aggregate" is a little artificial
but it demonstrates a useful point about not holding on to
references of "parent" (reference data) objects, i.e. the Visit has
an Integer petId, instead of a Pet field. In principle this app is
now almost ready to migrate to multiple services if anyone wanted
to do that.

Modularize and migrate to aggregate-oriented domain
Vet, Owner, Visit. The Visit "aggregate" is a little artificial
but it demonstrates a useful point about not holding on to
references of "parent" (reference data) objects, i.e. the Visit has
an Integer petId, instead of a Pet field. In principle this app is
now almost ready to migrate to multiple services if anyone wanted
to do that.
@vfedoriv

This comment has been minimized.

Show comment
Hide comment
@vfedoriv

vfedoriv Nov 11, 2016

Contributor

Hi.
As I see, you drop all service layer ?
IMHO, it's a not nice idea pull this in master branch.
Maybe, create new branch for microservices architecture ?

Contributor

vfedoriv commented Nov 11, 2016

Hi.
As I see, you drop all service layer ?
IMHO, it's a not nice idea pull this in master branch.
Maybe, create new branch for microservices architecture ?

@mszarlinski

This comment has been minimized.

Show comment
Hide comment
@mszarlinski

mszarlinski Nov 11, 2016

Hi @vfedoriv I am currently working on a microservices version and I named the branch "spring-cloud". Please see discussion in #198

I agree that this kind of refactoring should be a part of a microservices version.

mszarlinski commented Nov 11, 2016

Hi @vfedoriv I am currently working on a microservices version and I named the branch "spring-cloud". Please see discussion in #198

I agree that this kind of refactoring should be a part of a microservices version.

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Nov 11, 2016

Member

Clarification: this is not a "microservice" conversion. This is just a modernization, using Spring Boot and a clear modular structure in a way that is more consistent with best practice in 2016, as opposed to 2003 when this app was first written.

Member

dsyer commented Nov 11, 2016

Clarification: this is not a "microservice" conversion. This is just a modernization, using Spring Boot and a clear modular structure in a way that is more consistent with best practice in 2016, as opposed to 2003 when this app was first written.

@vfedoriv

This comment has been minimized.

Show comment
Hide comment
@vfedoriv

vfedoriv Nov 11, 2016

Contributor

"best practice" in Java - legacy support ;)

Contributor

vfedoriv commented Nov 11, 2016

"best practice" in Java - legacy support ;)

@verydapeng

+1, the service layer we had is quite thin, no much value added

@vfedoriv

This comment has been minimized.

Show comment
Hide comment
@vfedoriv

vfedoriv Nov 12, 2016

Contributor

Of course, you right.
But half of this project "a little artificial". :) How about model : BaseEntity -> NamedEntity ->...
We really need this ?

Contributor

vfedoriv commented Nov 12, 2016

Of course, you right.
But half of this project "a little artificial". :) How about model : BaseEntity -> NamedEntity ->...
We really need this ?

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Nov 12, 2016

Member

I agree those classes would have to be factored out or their features duplicated if you were to split it up into actual physical modules. My judgement is that this is a small enough task that I don't mind deferring it (probably it never need happen in this repo). Worth a comment in the README though.

Member

dsyer commented Nov 12, 2016

I agree those classes would have to be factored out or their features duplicated if you were to split it up into actual physical modules. My judgement is that this is a small enough task that I don't mind deferring it (probably it never need happen in this repo). Worth a comment in the README though.

@olivergierke

This comment has been minimized.

Show comment
Hide comment
@olivergierke

olivergierke Nov 14, 2016

Member

Should the VisitController move into the visit package, too?

Member

olivergierke commented Nov 14, 2016

Should the VisitController move into the visit package, too?

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Nov 14, 2016

Member

It's part of the "owner" UI, and as such it depends on the PetRepository. So moving it to the visit package creates a tangle.

Member

dsyer commented Nov 14, 2016

It's part of the "owner" UI, and as such it depends on the PetRepository. So moving it to the visit package creates a tangle.

@olivergierke

This comment has been minimized.

Show comment
Hide comment
@olivergierke

olivergierke Nov 14, 2016

Member

I wonder if it makes sense to invert (or break up) the relationship between Pet and Visit. I'd argue managing pets and owners is useful on its own (a CRM like application). The notion of visits to the doctor is then coming on top of that. Owners and pets without visits makes sense. A visit without a pet — not so much.

VisitRepository sort of already works that way as it exposes a findByPetId(…). This makes me feel like keeping the visits mapped on the Pet side is kind of obsolete and sort of breaking the abstraction direction.

So I'd rather make the visit an aggregate on it's own so that the Visit instances are created via the repository by being bound to a Pet (or it's identifier, I think even using Pet is fine here). That would allow us to move the URIs for view creation to the top level.

We could also go ahead and merge this as is so that we can create another PR with my suggested changes to see how that effects the structure of the codebase overall.

WDYT?

Member

olivergierke commented Nov 14, 2016

I wonder if it makes sense to invert (or break up) the relationship between Pet and Visit. I'd argue managing pets and owners is useful on its own (a CRM like application). The notion of visits to the doctor is then coming on top of that. Owners and pets without visits makes sense. A visit without a pet — not so much.

VisitRepository sort of already works that way as it exposes a findByPetId(…). This makes me feel like keeping the visits mapped on the Pet side is kind of obsolete and sort of breaking the abstraction direction.

So I'd rather make the visit an aggregate on it's own so that the Visit instances are created via the repository by being bound to a Pet (or it's identifier, I think even using Pet is fine here). That would allow us to move the URIs for view creation to the top level.

We could also go ahead and merge this as is so that we can create another PR with my suggested changes to see how that effects the structure of the codebase overall.

WDYT?

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Nov 14, 2016

Member

I played with various combinations and discarded them, but I'm not sure if that was one of them. Incremental changes after a merge also makes sense, if there are no objections to the change in principle (seems not).

Member

dsyer commented Nov 14, 2016

I played with various combinations and discarded them, but I'm not sure if that was one of them. Incremental changes after a merge also makes sense, if there are no objections to the change in principle (seems not).

@olivergierke

This comment has been minimized.

Show comment
Hide comment
@olivergierke

olivergierke Nov 14, 2016

Member

Okay, summarizing, I don't think anything in the proposed changes that really violates the fundamental goals of the ticket. So feel free to merge and I'll give it a spin on top for separate discussion.

Member

olivergierke commented Nov 14, 2016

Okay, summarizing, I don't think anything in the proposed changes that really violates the fundamental goals of the ticket. So feel free to merge and I'll give it a spin on top for separate discussion.

@dsyer

This comment has been minimized.

Show comment
Hide comment
@dsyer

dsyer Nov 14, 2016

Member

Merged 83ff9a5

Member

dsyer commented Nov 14, 2016

Merged 83ff9a5

@dsyer dsyer closed this Nov 14, 2016

@mszarlinski mszarlinski referenced this pull request in spring-petclinic/spring-petclinic-microservices Nov 22, 2016

Closed

Modernize Spring apps structure #16

@mszarlinski mszarlinski referenced this pull request in spring-petclinic/spring-petclinic-microservices Dec 5, 2016

Merged

closes #16 Modernize Spring apps structure #25

@arey arey referenced this pull request Dec 5, 2016

Closed

fix packages #215

@arey arey referenced this pull request in spring-petclinic/spring-petclinic-rest Feb 23, 2017

Closed

Migrate to Spring Boot #5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment