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

docs: ADR for 3 types of models #6436

Merged
merged 4 commits into from
Mar 12, 2024
Merged

docs: ADR for 3 types of models #6436

merged 4 commits into from
Mar 12, 2024

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Mar 5, 2024

About the changes

ADR documenting write model, external read model (complex read) and internal read model (simple read).

Important files

Discussion points

Copy link

vercel bot commented Mar 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 8, 2024 7:35am
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 8, 2024 7:35am

@@ -0,0 +1,39 @@
---
title: "ADR: Write Model vs Read Models"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! I was actually thinking about something related last night. We have developed the CRUD proof of concept that has been working quite well, but I thought about the interface segregation principle and how we could split that into WriteModel and ReadModel. Now making use of the dynamic typing, maybe we can easily setup smaller, tailor suited read models for different use cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd adjust the naming in the generic CRUD store to avoid ReadModel. What you really have in there is WriteModelInput/WriteModelDTO (which you called ReadModel) and WriteModel.

Also I try to avoid Interface Segregation Principle in this document because it's not a principle :) (https://dannorth.net/cupid-the-back-story/#interface-segregation-principle)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the names you suggest are much better, but everything becomes a Write+Something, so I just used ModelInput and ModelOutput which I think clearly reflects the intent of each type: #6500

@daveleek
Copy link
Contributor

daveleek commented Mar 7, 2024

This is great! I'm wondering about one thing though.

In the case that prompted this ADR and shift of mindset towards store/external read model/internal read model - the need to check if a segment is in use; how should we go about validating a command whose business rules rely on cross-domain piece of information?
Is this a separate domain (Segment usage) that gets its own internal read model?

Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on board but this current version is a bit too conceptual for me, so I would like to see some code examples on the ADR. Maybe a before/after so we can better grasp what this means in practice?

@kwasniew
Copy link
Contributor Author

kwasniew commented Mar 8, 2024

@daveleek so far we've been putting internal read model for cross domain information in the parent module (segment) that's closest to this information. So it's like lenses into the parent domain with minimal knowledge about it to only answer the question from this module.

@kwasniew
Copy link
Contributor Author

kwasniew commented Mar 8, 2024

@nunogois great suggestion. Added example section to the ADR

@nunogois
Copy link
Member

nunogois commented Mar 8, 2024

@nunogois great suggestion. Added example section to the ADR

Thanks!

Now that I looked at the examples and had some time to think about this I feel like I can better share my perspective. In my mind, each resource should have a single store. That store should be responsible for all DB-layer logic (essentially, queries) where that resource is the "main" resource. These resources have a service that is responsible for being the logical interface of the resource, including any other logic that may be necessary to manage the resource. Taking the segment resource as an example, in my mind it would look similar to what we have now:

  • segment-store.ts
  • segment-service.ts

When services access other resources, they should use the other resource service instead of the store directly, since there may be some additional logic that is critical to that resource but is not covered in the store. If possible, I wouldn't even provide direct access to the store from other services. Even if it looks like a simple segmentService.getAll() and its implementation a simple segmentStore.getAll() I think it should still go through the service.

So wherever you are in the codebase, if you need all segments, you know you need to do segmentService.getAll(). If you need all segments with their usage data, you do segmentService.getAllWithUsageData(). It's up to that resource service and store to decide how they do it, but the interface is very clear to me. Maybe the service calls other services for the extra data and does the mapping in-memory, or perhaps the store queries multiple tables. The caller does not care, it only cares about calling the correct method for its needs.

So I guess this is all to say that I don't see obvious advantages in having the different models when compared to just having different methods, each returning different types, all in the same place. It's still the same resource logically speaking and this provides easy and direct ways to it. It's also very simple on the store layer with the crud-store, since you only need to worry about adding methods for those more specific needs.

That being said, I'm still on board with this idea, I want to see how it will look like in our code base, trying it out and looking forward to the moment I better realize the advantages of doing things this way 🙂

@kwasniew
Copy link
Contributor Author

kwasniew commented Mar 8, 2024

@nunogois what about coupling between services. If module A needs to ask module B some question if we inject service from one service to the other we have coupling to ALL write methods from the other service even though you may not need them.

@nunogois
Copy link
Member

nunogois commented Mar 8, 2024

@nunogois what about coupling between services. If module A needs to ask module B some question if we inject service from one service to the other we have coupling to ALL write methods from the other service even though you may not need them.

I personally don't see that as a huge deal, but if we see value in that separation then I could see a read-store and write-store (or services, or models) being useful.

However having 2 different stores (or services, or models) for reads feels a bit blurrier to me.

@kwasniew
Copy link
Contributor Author

kwasniew commented Mar 8, 2024

@nunogois maybe using a different name for the two read models would help:

  • UI model - UI needs data from multiple domain where one domain is the primary one and you need to load all the other contextual data
  • decision model instead of internal read model - other module needs to read some data from another module to make a decision

The reason why I suggested two read models is they look very different. UI model/view model/external read model is usually very complex queries often taking 100 LOC or more. They make all the other code in the file almost invisible.
Decision model is usually very simple one table query often returning nothing but a list of strings.

So thinking in terms of responsibilities we have two different responsibilities:

  • display of data
  • making decisions
    The only thing they have in common is that in the SQL implementation they touch a database. However the question is if technology should drive our architecture or should responsibilities be the driver.

@nunogois
Copy link
Member

nunogois commented Mar 8, 2024

@nunogois maybe using a different name for the two read models would help:

  • UI model - UI needs data from multiple domain where one domain is the primary one and you need to load all the other contextual data
  • decision model instead of internal read model - other module needs to read some data from another module to make a decision

The reason why I suggested two read models is they look very different. UI model/view model/external read model is usually very complex queries often taking 100 LOC or more. They make all the other code in the file almost invisible. Decision model is usually very simple one table query often returning nothing but a list of strings.

So thinking in terms of responsibilities we have two different responsibilities:

  • display of data
  • making decisions

I may be wrong here but I think that line is a bit blurry for most of our resources, where the data we see on the UI is the same as the data we fetch for decisions. It's usually the method we have on the base GET method of that resource.

If we're using the CRUDStore and need a specific, more complex query for a view, then IMO it belongs in the methods we add to that specific CRUDStore. It's probably going to be the only, or one of the very few, methods there anyways, since CRUDStore should cover the common ones.

Alternatively, if we face this issue often, or if the queries are that complex, maybe we should consider creating views in the database, or rethink our database structure.

The only thing they have in common is that in the SQL implementation they touch a database. However the question is if technology should drive our architecture or should responsibilities be the driver.

I see it as "resources" instead, similar to our new feature-driven project structure. If I want X, I'll fetch it from the XService, and my decision of the kind of X I want and how i want it is done by the method that I choose.

Despite my current perspective being different I don't have strong feelings about it. Like I said I'm willing to try this approach and I'm always willing to learn better ways of doing things 🙌

gastonfournier added a commit that referenced this pull request Mar 11, 2024
## About the changes
Just renaming to bring more clarity based on
#6436 (comment)
@kwasniew kwasniew merged commit 74df643 into main Mar 12, 2024
14 checks passed
@kwasniew kwasniew deleted the adr-3-types-of-models branch March 12, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants