-
Notifications
You must be signed in to change notification settings - Fork 0
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
20 create a projection showing the current balance of an account #45
20 create a projection showing the current balance of an account #45
Conversation
Add entity and domain class. Add ports and jpa adapter
Fix table annotation on account entity. Add getAccountBalance implementation and new ports
…-the-current-balance-of-an-account # Conflicts: # demo-transactions/src/main/kotlin/io/holixon/cqrshexagonaldemo/demoparent/transactions/adapter/inbound/account/AccountRestInAdapter.kt # demo-transactions/src/test/kotlin/io/holixon/cqrshexagonaldemo/demoparent/transactions/integrationtest/GlobalIT.kt
…be reusable for negative values (negative balance of the account). Validation of amount rules in useCase.
add schema of read database to flyway Use root user on database for now.
Refactor: Add 2 flyway migration schemas for each database. Refactor code.
The current package splitting confuses me a bit. Than all 1st level packages (command, query and shared, while root is level zero) can have the typical separation into domain, application and infrastructure again. |
"Amount must be positive" | ||
} | ||
} | ||
|
||
val account = accountOutPort.findAccount(iban) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I really don't like the naming, that we introduced here in the beginning. Reading "InPort" and "OutPort" in the usecase doesn't feel right, since it doesn't help to understand what is happening, but distracts by bringing "reading complexity".
I would like to have those things named by what they are meant to do, than by what they are. Like AccountService
or AccountRepo
instead of AccountOutPort
. We can add OutPort
and InPort
annotations to the framework
package, and mark the port-interfaces with those annotations, but that should be it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you mean and i will do the change. The outPort could be in fact called Services like AccountService instead of AccountOutPort. but i think the inPort could be kept no ? Since InPort immediately tells you what its purpose is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do it unison - name it after its domain/application purpose and add the architectural purpose as annotation. But this is not for now, we can do that later.
@@ -0,0 +1,5 @@ | |||
CREATE TABLE `cqrs-meets-hexagonal-read-db`.account_balance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it add any confusion to on the one side use the wording 'read' and 'write' (within db resources) and on the other side 'command' and 'query' (in package structures)?
Maybe we should stick to command-model
and query-model
instead of write and read (especially since on both sides data is writted and read).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i changed to a better suited name, which means that you need to clear the database.
Refactor: Rename outPorts to services for better readability Refactor: Moved packages to match hexagonal architecture pattern.
@@ -33,9 +33,13 @@ spring: | |||
generate_statistics: true | |||
database-platform: org.hibernate.dialect.MySQL8Dialect | |||
flyway: | |||
enabled: true | |||
enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not enabled? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because i added .../shared/infrastructure/database/flyway/FlywayConfig.kt
file that creates 2 flyway migration schemas, and i can't use flyway enable since it overrides the other. With 2 flyway schemas we can have V0_0_0 in query and also V0_0_0 in command. So we complete split the 2 databases migration schemas. i think this as also an ideia that patrik added on the last EDM event tell me what you think 🙈
flyway/local.conf
Outdated
flyway.url=jdbc:mysql://localhost:3306/ | ||
flyway.locations=classpath:db | ||
flyway.locations=classpath:db/read,classpath:db/write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the same here: query
and command
instead of read
and write
private lateinit var accountService: AccountService | ||
|
||
//@Autowired | ||
//private lateinit var accountBalanceInPort: AccountBalanceInPort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better removed then commented out :-)
This PR aims to refactor the package structure has discussed on EDM(Event-driven-microservice) day with suggestions:
Ps: On the next issue #21 the linking between the sync of the balance with read model will be done.