⚠️ Updating our folder structure

[mikededo]

🗃️ Changes to our file structure

📝 Description

This last class we discussed some issues with the teacher and I came to the conclusion that we do not have a proper folder structure, and that there are some things that have to be changed. This also affect the current PR in progress which will add the database controller (see #102).

❗ Problem

The problem focuses in the io.packland.mdas.githubstats package.

The entrypoint of our application (as of now) is the infrastructure.shell package. This package has, as of now, one controller, the UserOptionController that currently is responsible of handling the execution when the user option is selected. It creates the required GitHubRepositories, which are implemented under infrastructure.github.repository and it handles all the logic.

The bad decision in this case is that we directly coupled the controller to the GitHub implementation. This means that, if by any chance we allowed the user to choose if they want to fetch the data from GitHub or GitLab, we would not be able to reuse the current UserOptionController, since it is tightly coupled with the GitHub REST API implementation.

The same happens with the handling of the data from the database: it is tightly coupled to the PostgreSQL implementation.

💡 Proposal

I want to remark that this approach is completely opinionated. However, I think it is a solution that allows us to extend the behaviour to many databases or any external service.

🌐 GitHub - External services

First, we have to decouple the UserOptionController from the GitHub REST API. In order to do this, we have to:

1. Create a new package inside infrastructure.github named controller that contains the logic of the UserOptionController coupled to the GitHub REST API. In this case this is not an issue since it is inside the infrastructure.github package, and it has to be coupled to such implementation.
2. The controller from the infrastructure.shell.controller that handles the UserOption will have to call the GithubUserOptionController which will run the code that fetches data from the GitHub API.
3. When fetching data from an external service, we have to store such data. At the same time, we must avoid letting the infrastructure.github know about the database we are using. With @manerow, we thought about injecting the GithubUserOptionController with a use case that will allow the controller to save the data. Furthermore, thanks to JPA, we would only need to save an organization, as it will cascade save the other entities.

If there's ever the need to add a new external service, for example fetching data from the GitLab API, we would just have to replicate the package structure from infrastructure.github (implementing the required repositories for the use cases that it uses and the GitLabUserOptionController). Then, inside the UserOptionController inside the infrastructure.shell package, we would choose, depending on the user input, one controller or the other. Here's an example:

# user calls github user option
user -n mikededo -k aKey --rest github --from 10/22 --to 11/22
# inside the UserOptionController of the shell package, we would execute the
# GithubUserOptionController because the github option was chosen

# user calls gitlab user option
user -n mikededo -k aKey --rest gitlab --from 10/22 --to 11/22
# here, it will run the GitLabUserOptionController instead

# same logic would apply for other services (i.e. BitBucket)

💾 Database

A similar thing happens with the databases. We currently have a PostgreSQL database, but maybe in a future we add a microservices structure and user's data is stored in a PostgreSQL database while team's data is stored in a MySQL database. Our incoming implementation (#102) does not differentiate the database that we are using since it adds everything inside the infrastructure.controller folder.
The solution would be similar to the external services approach:

1. We would define a new pacakge inside infrastructure for each database system. For example, we would now move the infrastructure.controller package inside infrastructure.postgres package.
2. Inside our database package, we would implement the repositories that are required for our use case (most likely we will not have, as they are implemented by JPA). We will also implement the controllers for the different options that our application has or that we want our database pacakge to be able to have. For example, the PostgreslUserOptionController and PostgresTeamOptionController.
3. Then, inside our infrastructure.shell package, we would decide on which database system we choose regarding the user situation.

🗺️ DTO's and mappers

Another issue that we have is regarding our entities and our DTO's. Initially, we had defined DTO's inside the application.dto package. Furthermore, we had implemented the mappers that would convert the DTO to a domain entity and vice-versa (if needed of course). However, we found ourself working both with DTO's and domain entities inside the controllers, which was the reason why we decided to move our JSON translation (mapping) to our entities inside the domain layer. With this, we were also excluding the need of mappers

This was a terrible decision as we were coupling information that should only be know on the infrastructure layer, inside our domain layer. This is breaking all the rules about our intend to apply hexagonal architecture to the project.

Now, in order to fix this, we have to implement the DTO's outside the domain layer (obviously) into our infrastructure layer. The reason on why I think that the DTO's should be inside the infrastructure layer is because the JSON mapping will depend on the REST API impementation and therefore the JsonProperty annotation that we are using will be different for every service that we are using. Here's an example:

// The schemas displayed here are completely made up!
//
// Let's say that to fetch pull requests from GitHub, we receive as a response a JSON like:
// [
//   {
//     "id": 1,
//     "title": "feat: add our new controller",
//     "author": "mikededo"
//   }
// ]
// 
// However, if we fetch pull requests from GitLab, we receive as a response:
// [
//   {
//     "pr_id": 1,
//     "title": "feat: add our new controller",
//     "creator": "mikededo"
//   }
// ]
//
// We can obviously see that both implementations are different and therefore, we should
// define different dto's for each case:
// infrastructure.github.dto
public class GitHubPullRequestDTO {
    @JsonProperty("id") Integer id;
    @JsonProperty("title") String title;
    @JsonProperty("author") String author;
}
// infrastructure.gitlab.dto
public class GitLabPullRequestDTO {
    @JsonProperty("pr_id") Integer prId;
    @JsonProperty("title") String title;
    @JsonProperty("creator") String creator;
}

By creating different classes we allow ourselves the possibility of having different JsonProperty assignations, simplifying the logic.

With this decision, another problem arises and it is the mappers. For mappers, we could also have the same approach and have a GitHubPullRequestMapper and a GitLabPullRequestMapper which will convert the DTO's to the expected entity.
However, I believe that we can take a better approach and define a contract for the DTO's. This contract will be used inside the mappers to create the entity without knowing the specific implementation of the DTO. Let me first explain my steps and later I will provide an example:

1. We will have a PullRequestDTO interface that will define a set of methods (getters) that the specific implementation for the DTO will have to implement. This methods will be used to obtain data from the DTO.
2. The concrete DTO (GitLabPullRequestDTO or GitHubPullRequestDTO) will implement such DTO contract.
3. The mapper will receive a an argument of the PullRequestDTO and it will convert such argument to a PullRequest entity.

The exemple would be, using the previously defined DTO's:

// First we define the contract
// application.dto
public interface PullRequestDTO {
    public Integer getId();
    public String getName();
    public Stirng getAuthor();
}
// We would change the signature of our DTO's to the following and we would
// implement the interface methods
// infrastructure.github.dto
public class GitHubPullRequestDTO implements PullRequestDTO {
    // ...
}
// infrastructure.gitlab.dto
public class GitLabPullRequestDTO implements PullRequestDTO {
    // ...
}
// Finally, we would only need our PullRequest mapper
// application.mapper
public class PullRequestMapper {
    public static PullRequest dtoToEntity(PullRequestDTO) {
        // ...
    }
}

🎓 Conclusion

After all that I have discussed, if we all agree, we should come up with a solution/decision and start migrating/updating our code to the decided solution! ✌🏼

The folder structure, after applying the changes, would look as:

io/packland/mdas/githubstats/
├── application/
│   ├── exceptions
│   ├── dto
│   ├── mapper
│   └── ...
├── domain/
│   ├── exceptions
│   ├── repository
│   └── ...
└── infrastructure/
    ├── github/
    │   ├── controller
    │   ├── dto
    │   └── repository
    ├── postgres/
    │   └── controller
    └── shell/
        ├── components
        ├── configuration
        ├── controller
        ├── model
        └── validation

[sdomingobasora]

As a first feedback I think your solution looks great!

For me though, the premise of Github being at the very core of the application and coupled with implementations didn't sound so bad, because at the end it was very central to the purpose of the project and even its name. If we wanted to get data from other sources like Gitlab, we would have to even change the model itself (different concepts merge requests vs pull requests...) and the application would be different in its objectives.

But if we look at it from a extensibility standpoint, it makes sense to be able to adapt to different sources. At the end, we don't know how much the requirements could change in the future, so it's nice to have.

I'll have a deeper look later!

[manerow]

After reading the article I also agree that the @mikededo proposal looks great! Nevertheless, while i share same opinion on some aspects I don't quite see the approach of the PostgresqlController and the placement of DTO's at the Infrastructure layer.

Under my point of view, there shouldn't be a PostgresUserOptionController replicating the logic of fetching data from the External APIs (GitHub in this case), since while doing this we still have the need of coupling the GitHubUserOptionController with an specific DB system for storing the data we fetch. On the other hand, I find that having a controller of controllers dont match with the clean architecture principles and its onion layer design of outer layers calling innermost ones.
We should definitely aim to decouple our usecases from the specific implementation of our repositories and for doing so we should rely on the ports and adapters pattern. Declaring the repository ports on our use case constructors and instantiating specific adapter implementations in the Controller which calls the use case.

When the user introduces the following sequence of commands:
user -n mikededo -k aKey --rest github --from 10/22 --to 11/22
The UserOptionComponent of the shell infrastructure package already has all the information to decide which controller has to be called. For instance, the GitHubUserOptionController. And I think that we should assume by design that the information from GitHub is currently stored in a Postgres DB system. So in the GitHubUserOptionController we should decide if we fetch the information from GitHub or from Postgres, and provide PostgresRepository adapter instances to the usecases which retreive information from DB and GithubRepository instances to the use cases that fetch information from External APIs.

The point of hexagonal design on here for me would be if we change the project design and decide that GitHub data is stored on MySQL. Then, since we used the ports and adapters approach, we would only have to change the repository adapters which are passed to the usecase constructors in the GitHubUserOptionController and implement those adapters for the MySql logic.

If in the future we are asked to develop a feature that can handle:
user -n mikededo -k aKey --rest gitlab --from 10/22 --to 11/22
We still can extend our implementation by creating a GitLabUserOptionController. And if we decide that GitLab stores data on MySQL, we still can reuse all the usecases by providing MySqlRepository adapter instances to those usecases that retreive data from DB and GitLabRepository instances to those that fetch information from External APIs.

About the DTOs placement, i think that DTOs sould belong to the application layer, as they shouldn't be coupled to the db/external repository implementations but to the use cases we want to call from the Controllers.

Notice that this is my point of view and I completely agree to discuss it all on a meeting later. Thank you @mikededo for your in-depth analysis.

[mikededo]

Under my point of view, there shouldn't be a PostgresUserOptionController replicating the logic of fetching data from the External APIs (GitHub in this case), since while doing this we still have the need of coupling the GitHubUserOptionController with an specific DB system for storing the data we fetch.

If I made myself understand that I wanted to create a PostgresUserOptionController, then I did not explain myself correctly.

The PostgresUserOptionController will only contain the implementation to retrieve information from the database. In any case it would be responsible for fetching any data from any external service.

The difference between the PostgresUserOptionController and the GitHubUserOptionController is that one will obtain data from an internal service (which means that it will make use of the UC that provide such utility) and that the other will fetch data from an external service (using the UC that provide such utility).

On the other hand, I find that having a controller of controllers dont match with the clean architecture principles and its onion layer design of outer layers calling innermost ones.

True, but every application requires an entrypoint. Imagine that we have a REST application and the code of this app follows a clean code approach. The server will receive HTTP requests to its entrypoint and will let the controllers handle them. Now, is this entrypoint a controller?
This is the same thing that happens to us. The infrastructure.shell is a controller, but a the same time is the entrypoint to our app, which will have to delegate the tasks to other parts of the application.

For instance, the GitHubUserOptionController. And I think that we should assume by design that the information from GitHub is currently stored in a Postgres DB system.

We could do that. We would have to move the postgres implementation inside the infrastructure.github package, though.

About the DTOs placement, i think that DTOs sould belong to the application layer, as they shouldn't be coupled to the db/external repository implementations but to the use cases we want to call from the Controllers.

Ok. Let's think we take this approach. How would you solve the problem I have described between the differences of JSON schemas that we may receive from the different responses? @plozanol, since you have said something similar, I would like to know what your solution would be.

The only way I see of solving this issue is by setting a contract of the DTO, whichi of course is service-agnostic and should be placed in the domain layer.

[plozanol]

In first place kudos to @mikededo for such great analysis and the initiative to post the discussion. There are good points from all of you. I'll try to add mine.

In the current context and knowledge, we all agree that doesn't make sense the proposal I made with moving GitHub and DB controllers to infrastructure.controllers.

Every infrastructure has to have its own controllers, and as @manerow has noted it doesn't make sense to overcomplicate and make a controller of controllers.

We all agree that GitHub and DB controllers implement the same logic to retrieve data, not the implementation detail. We should make a contract that both pieces must implement. GitHub controller has a difference: it also has to persist the data when fetching is finished.

Mixing the hierarchy proposed by @mikededo, I would like to add my view on how it should be, I'll mark with ** the additions

githubstats
  ├── application
  │ ├── exceptions
  │ ├── discount
  │ ├── mapper
  │ └── ** use case
  │ ├── ** fetch
  │ ├── ** get
  │ └── ** save
  ├── domain
  │ ├── exceptions
  │ ├── repository
  │ ├── ** entity
  │ └── ** services
  └── infrastructure
      ├── github
      │ ├── controller
      │ └── repository
      ├── postgres
      │ ├── ** repository
      │ └── controller
      └── shell
         ├── components
         ├── settings
         ├── controller
         ├── model
         └── validation

I will start with easy throughs about this:

• application.usecase (and subfolders) isn't really needed, just seems cleaner visually to me.
• domain should have a service folder, containing all the use cases that are intermediaries, because at the application layer only should be the main use cases that characterize our application.
• domain should have an entity folder, is just more organized.

At this point of the project we have set a basis of code, and we don't need any more enablers to rapid-develop. I'm referencing JPA and lombok:

• JPA generates the boilerplate CRUD code at an abstraction level. Therefore, it isn't correct to add JPA repositories to the domain repository, because it already contains an implementation generated with the JPA settings provided (Postgres driver).
  Also, I don't like to inherit lots of methods from JPA Repository, that we don't need and break the clean design and principles we are looking at: https://betterjavacode.com/wp-content/uploads/2021/04/JPARepositories.jpg

I propose that, domain.repository creates the abstract contract of the database interface, and at the infrastructure, the Postgres implementation uses SQL queries. This way it will be more flexible to have more than one implementation of the persistence layer.

At GitHub - External services point 3, @mikededo suggestes applying the dependency inversion principle and injecting the Repository interface and is totally right. Here I just to note that we need to set bidirectional relations manually when we fetch data from Github, because when persisting an entity if its integrity doesn't match, the entities will not be relational. This is not currently done.

• lombok: it is very handy, but also breaks easily the encapsulation principle, implementing getters and setters with @data annotation and others. We should refactor these and only create what we need, in favor of using tell don't ask always. Sure we will generate boilerplate code, but only the needed, and always control it.

We need to create a Proxy design pattern, that should be called by the shell, and it will encapsulate the logic to decide which collaborator to call (internal, external) in the function if the request is already at the historic table.

What @mikededo said about DTO and mappers is right: entities are dirty with plenty of mixing of annotations from persistence (@Column, @Entity, etc..) layer and from infrastructure layer (@JsonProperty). It will be cleaner to separate the concepts. In this way, I also suggest implementing Value Objects in order to obtain Immutability and Classes with their own Exceptions, as some already do. Value will allow us to encapsulate validation logic too.

DTOs are coupled to the implementation, and their name should reflect it too. @sdomingobasora is right when saying this app is github-stats and isn't our objective to have flexibility in this matter. We don't need to overcomplicate without specific requirements.

But also I agree with @manerow about the placement of DTOs should be at the application layer, because they are not domain entities, just representations that will be used with the superior layer (infrastructure), and therefore needs to be close to it to not violate the hexagonal mindset.

[mikededo]

We all agree that GitHub and DB controllers implement the same logic to retrieve data, not the implementation detail. We should make a contract that both pieces must implement. GitHub controller has a difference: it also has to persist the data when fetching is finished.

Now I realise that I haven't explained myself properly! 😢

I propose that, domain.repository creates the abstract contract of the database interface, and at the infrastructure, the Postgres implementation uses SQL queries. This way it will be more flexible to have more than one implementation of the persistence layer.

I agree with that. Do you want to take charge of it, persistance team (@plozanol, @sdomingobasora, @cjp4)? 😆

Here I just to note that we need to set bidirectional relations manually when we fetch data from Github, because when persisting an entity if its integrity doesn't match, the entities will not be relational.

True, we have started to implement it again (see Team::addRepository). This should be added as the necessity appears.

In this way, I also suggest implementing Value Objects in order to obtain Immutability and Classes with their own Exceptions, as some already do. Value will allow us to encapsulate validation logic too.

I do not really understand the usage of VO here and how would they help us in solving anything (what's the problem we are trying to solve with VO?).

But also I agree with @manerow about the placement of DTOs should be at the application layer, because they are not domain entities, just representations that will be used with the superior layer (infrastructure), and therefore needs to be close to it to not violate the hexagonal mindset.

Regarding this, I have tagged you in a reply to @manerow's answer.