fix(188): code review #193
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
updating pom file to add build and properties
adding apache commons lang 3 as a dependency
refactoring a small portion of the code to: - comply with checkstyle guideline - use records when possible - removed interfaces for services
removed a few dependencies as they are not required: - jakarta.inject-api - commons-collections - jettison - spring-context-indexer Fix #188
Entity classes are supposed to be session-bounded. By annotating a class with @component you're flagging it for spring to instantiate it as a managed bean, keeping it as a singleton. As an entity is supposed to be just a data holder for the database, it makes no sense to use it as a component. The scope controls the component scope or in simple words, marks how spring will control this singleton lifecycle. You can set a scope per user session (meaning that a bean will exist for each user session) or more global. Fix #188
Entity classes are supposed to be session-bounded. By annotating a class with @component you're flagging it for spring to instantiate it as a managed bean, keeping it as a singleton. As an entity is supposed to be just a data holder for the database, it makes no sense to use it as a component. The scope controls the component scope or in simple words, marks how spring will control this singleton lifecycle. You can set a scope per user session (meaning that a bean will exist for each user session) or more global. Fix #188
Closed
|
[nr-forest-client-backend] SonarCloud Quality Gate failed. |
adding lombok as a dependency and refactoring the code to make use of it. Lombok reduces the boilerplate code by annotating classes that will generate the actual content during build time.
removed `Serializable` interface from entities. Even being a best practice to mark entities as serializable, you only need to mark it if you need to transfer them over-the-wire (serialize them to some other representation), store them in http session (which is in turn serialized to hard disk by the servlet container), or even persist it as a binary content.
as stated on JPA specification:
If an entity instance is to be passed by value as a detached object (e.g., through a remote interface), the entity class must implement the Serializable interface.
in our case, hibernate will pass it as a reference, and update the fields on the database even if we do not set it as `Serializable`
Fix #188
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 18.11.12 to 18.11.15. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Derek Roberts <[email protected]> Co-authored-by: MCatherine <[email protected]>
#186) Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.46.0 to 5.46.1. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.46.1/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat/frontend: added check duplicate api to frontend, refs: #190 * feat/frontend: reinstalled node packages, refs: #190 * feat/pipeline: adjust memory request for deployment yaml file * feat/frontend: added example for generate form based on json schema, refs: #50 * Exclude example dir from SonarCloud * Comment out placeholder file Co-authored-by: Derek Roberts <[email protected]>
refactoring a small portion of the code to: - comply with checkstyle guideline - use records when possible - removed interfaces for services
removed a few dependencies as they are not required: - jakarta.inject-api - commons-collections - jettison - spring-context-indexer Fix #188
adding lombok as a dependency and refactoring the code to make use of it. Lombok reduces the boilerplate code by annotating classes that will generate the actual content during build time.
removed `Serializable` interface from entities. Even being a best practice to mark entities as serializable, you only need to mark it if you need to transfer them over-the-wire (serialize them to some other representation), store them in http session (which is in turn serialized to hard disk by the servlet container), or even persist it as a binary content.
as stated on JPA specification:
If an entity instance is to be passed by value as a detached object (e.g., through a remote interface), the entity class must implement the Serializable interface.
in our case, hibernate will pass it as a reference, and update the fields on the database even if we do not set it as `Serializable`
Fix #188
adding lombok as a dependency and refactoring the code to make use of it. Lombok reduces the boilerplate code by annotating classes that will generate the actual content during build time.
removed `Serializable` interface from entities. Even being a best practice to mark entities as serializable, you only need to mark it if you need to transfer them over-the-wire (serialize them to some other representation), store them in http session (which is in turn serialized to hard disk by the servlet container), or even persist it as a binary content.
as stated on JPA specification:
If an entity instance is to be passed by value as a detached object (e.g., through a remote interface), the entity class must implement the Serializable interface.
in our case, hibernate will pass it as a reference, and update the fields on the database even if we do not set it as `Serializable`
Fix #188
removing SwaggerConfiguration.java as it is not required anymore. updating OraclePersistenceConfiguration and PostgresPersistenceConfiguration for better maintainability. moved configurations to reside inside YML file and reused properties file for build purpose. added documentation folder with initial compose file.
github-actions
bot
added
fix
and removed
documentation
github-actions
bot
added
documentation
| import java.util.Date; | ||
|
|
||
|
|
||
| public record CodeDescrDTO( |
There was a problem hiding this comment.
@paulushcgcj let's please use the suffix Dto instead of DTO. I know it is an acronym, but let's please use Dto.
github-actions
bot
added
documentation
|
[forest-client-frontend] Kudos, SonarCloud Quality Gate passed!
|
MCatherine1994
approved these changes
Jan 5, 2023
… fix/188-code-review
github-actions
bot
added
documentation
github-actions
bot
added
documentation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change is a code review. It is only to make the application more compliant with java and spring guidelines. We are removing some unused dependencies, aligning code and refactoring the code to look more like a Java 17 application.
The decision to move from MVC to WebFlux is based on two reasons:
1 - With webflux, you can achieve a higher throughput due to the way reactive components work (they reuse unused threads to process other bits of code while they wait for other processes to respond).
2 - The code itself resembles more a Typescript/Javascript promises code, something that's common when writing frontend code, making it easier to jump from one codebase to another.
Another benefit is the similarities between an expressJS route and a functional route on webflux, the ability to provide retry, backpressure and rate limiting on the processing stream.
In terms of folder structure changes, the code was changed to accommodate an approach that split the code by areas, instead of modules. It will make it easier to navigate the same kind of code (such as services or endpoints) inside the same folder structure than it is when splitting by modules. The module structure is usually associated with enterprise java applications (AKA EJB), something more aligned with a monolithic codebase, where this kind of organization makes more sense.
Some of the ideas applied with the proposed changes are aiming into moving towards a microservice approach, where those internal modules would become their own services, with their own codebase. The reactive code would then be used and some of its mechanisms, such as backpressure and retry can then be used to accommodate failures in a seamless way.
Fixes #188
Type of change
How Has This Been Tested?
For the time being, the existing test classes were maintained and no new scenario was included.
Checklist
Further comments
Most of the changes were made to simplify the code. It is a lot of changes but only aesthetic.
Thanks for the PR!
Any successful deployments (not always required) will be available below.
Backend available
Frontend available
Once merged, code will be promoted and handed off to following workflow run.
Main Merge Workflow
Thanks for the PR!
Any successful deployments (not always required) will be available below.
Backend available
Frontend available
Once merged, code will be promoted and handed off to following workflow run.
Main Merge Workflow
Thanks for the PR!
Any successful deployments (not always required) will be available below.
Backend available
Frontend available
Once merged, code will be promoted and handed off to following workflow run.
Main Merge Workflow