Feat/poc add dao layer #2248
Feat/poc add dao layer #2248
Conversation
…mulatorTeam/AntaREST into feat/poc-add-dao-layer # Conflicts: # antarest/study/business/link/CompositeLinkDAO.py # antarest/study/business/link/LinkDAO.py # antarest/study/business/link/LinkFromStorageDAO.py
There was a problem hiding this comment.
A general comment:
I think we should isolate all the DAO layer in its own package (I propose antarest.study.dao).
This DAO layer must be completely independent from the "business" layer which will use it.
The dependency must go only from the business layer to the DAO layer, not the other way around, so we should not mix them in the same package.
| List[LinkDTO]: A list of all links associated with the study. | ||
| """ | ||
| links = self.cache_dao.get_all_links(study) | ||
| if not links: |
There was a problem hiding this comment.
There is a difficulty with using the LinkDAO interface for the cache:
we cannot really know if the data is not in cache, or if it's just that the data is an empty list.
We should have an Optional instead, but we don't want to introduce those optionals in the interface just for the need of the cache.
So we are left with 2 options I think:
- either have a specific interface for the cache, with optionals
- or use the approach of the other PoC branch where the
CacheDAOimplementation can check itself if the data is in the cache or not
There was a problem hiding this comment.
Or maybe a third alternative !
- the cache implementation could throw a
CacheMissError, that we will catch here
It will rely on a quite "hidden" convention but why not ! Relying on exceptions is quite pythonic from what I know ...
| redis (ICache): The cache interface used for storing and retrieving links. | ||
| """ | ||
|
|
||
| def __init__(self, redis: ICache): |
There was a problem hiding this comment.
We should not name it redis, the cache implementation is not always based on redis
| command_context=self.storage_service.variant_study_service.command_factory.command_context, | ||
| study_version=file_study.config.version, | ||
| ) | ||
| execute_or_add_commands(study, file_study, [command], self.storage_service) |
There was a problem hiding this comment.
In the end the DAO layer should not use the commands, it's the opposite:
the commands should use the DAO layer.
Otherwise, we will not be able to change the underlying storage without changing the commands implementation.
| LinkDTO: The link that was added. | ||
| """ | ||
| self.storage_dao.create_link(study, link_dto) | ||
| self.cache_dao.create_link(study, link_dto) |
There was a problem hiding this comment.
I am not sure it's a good idea that the cache implementation has to implement the modifications methods:
i can be error prone because we may need to implement duplicate logic between the "storage" implementation and the "cache" implementation, and ensure they are consistent.
In general, we populate a cache from what we read from the underlying storage, so we could just do nothing with the cache here except invalidate it (remove the link from the cache). Then on the next read it would be put in the cache again.
There will be a small performance loss but it's probably not too bad, at least in a first implementation, compared to the risk of error we introduce by duplicating the modification logic
| if not links: | ||
| links = self.storage_dao.get_all_links(study) | ||
| for link in links: | ||
| self.cache_dao.create_link(study, link) |
There was a problem hiding this comment.
I don't think we should use the create_link method here, it's not the same thing to actually create a link and to just put an existing link in the cache.
| self.cache_dao = cache_dao | ||
| self.storage_dao = storage_dao | ||
|
|
||
| def get_all_links(self, study: Study) -> List[LinkDTO]: |
There was a problem hiding this comment.
about naming: I think we should reserve DTO for the web layer. The DAO layer should have its own suffix, for example Data.
POC on link handling to test a new architecture. While currently limited to links, it can be extended in the future.
Refactoring of DAO Layer
CompositeLinkDAO, which coordinates interactions between:LinkFromCacheDAO: Handles cached data.LinkFromStorageDAO: Manages data in persistent storage.LinkFromCacheDAOandLinkFromStorageDAO) now implement the commonLinkDAOinterface, ensuring a unified API for link management.Updated API and Service Layers
LinkDTO, removing any dependencies on internal models.LinkDTOobjects into internal models when interacting with the DAOs, ensuring modularity.Improved Architecture
CompositeLinkDAOfor delegating tasks to cache and storage layers.Architecture Diagram
graph TD A[API<br>Receives requests<br>and returns DTOs] A --> B[Service<br>Business logic,<br>delegates to DAO] B --> C[Composite DAO<br>Inherits from LinkDAO<br>Delegates tasks to:<br>- LinkFromCacheDAO<br>- LinkFromStorageDAO] C --> D[LinkFromCacheDAO<br>Implements LinkDAO<br>Interacts with the caching system] C --> E[LinkFromStorageDAO<br>Implements LinkDAO<br>Interacts with persistent storage]