colinbankier / realworld-tide

Rust / Tide example app following realworld.io
149 stars 26 forks source link

Domain modelling #19

Closed LukeMathWalker closed 4 years ago

LukeMathWalker commented 4 years ago

The main purpose of this PR/huge refactor is to properly isolate:

I have also added somewhat robust error handling to the domain layer as well as fixing a bunch of bugs here.

I have added myself to the list of authors for the crates in the workspace for attribution - let me know if this is ok with you @colinbankier.

Outstanding:

colinbankier commented 4 years ago

Thanks @LukeMathWalker - this is looking great. I've only had a superficial scan through so far, but will try take a deeper look soon. While I like providing some good structure (particularly in an explicitly "example" app) - I was hesitant to add too many layers of abstraction unnecessarily. However - seeing your comment
"If you want to get a taste of what it feels like to build a Realworld's implementation using another Rust's web framework, you can reuse the domain and db sub-crate." - I think this is a great idea.

Also "I have added myself to the list of authors for the crates in the workspace for attribution" - totally fine - you probably have added more lines than me by now :)

There seems to be some clippy CI failure - and I'll take a deeper look through soon :+1:

LukeMathWalker commented 4 years ago

While I like providing some good structure (particularly in an explicitly "example" app) - I was hesitant to add too many layers of abstraction unnecessarily. However - seeing your comment "If you want to get a taste of what it feels like to build a Realworld's implementation using another Rust's web framework, you can reuse the domain and db sub-crate." - I think this is a great idea.

Yeah, I agree there is always a fine balance between too much or not enough abstraction, but I think it makes sense to make the heavy lifting work we have done here re-usable. Another angle would be testing out an ORM replacing the db sub-crate, which I'd actually be really curious to see for some of the upcoming new crates (e.g. sqlx).

There seems to be some clippy CI failure - and I'll take a deeper look through soon

They should be fixed now :+1:

LukeMathWalker commented 4 years ago

Currently the web layer directly connects to the db layer (since the concrete implementation of repository is in db)

The web does not refer to that concrete implementation (or the db crate at all) - it's only in application that we are actually passing in the concrete repository implementation.

here is no real application "core" for business logic that is distinct from db access logic. It feels like the logic in db::repository is actually "core" logic, and the actual interface to db could be more what is defined in db::queries.

I agree that the application is a bit skinny when it comes to core logic - but I think the boundary is correct, because the actual Repository trait has to live in domain. I do agree though that we probably have a couple of methods in Repository that are doing too much beyond data mapping between ORM schemas and domain - we should probably move part of that logic in domain :+1:

The other thing we could do is move the pieces of logic inside the web handlers that use repository s commands and call methods on domain object in the actual domain sub-crate, thus reducing the web crate to the bare minimum HTTP-related functionality.

colinbankier commented 4 years ago

:+1: Merged - let's continue further refactorings from there