[PHEE-684] Reporting CRUD APIs - #120
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
| # password: "ethieTieCh8ahv" | ||
| # password: lavanya | ||
| password: "ethieTieCh8ahv" | ||
| common: |
There was a problem hiding this comment.
Security control: Secret Detection
Type: Hashicorp-Tf-Password
Description: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
Severity: HIGH
Jit Bot commands and options (e.g., ignore issue)
You can trigger Jit actions by commenting on this PR review:
#jit_ignore_fpIgnore and mark this specific single instance of finding as “False Positive”#jit_ignore_acceptIgnore and mark this specific single instance of finding as “Accept Risk”#jit_undo_ignoreUndo ignore command
| import lombok.*; | ||
|
|
||
| import javax.persistence.*; | ||
|
|
There was a problem hiding this comment.
Using wildcards is a bad practice.
|
|
||
| public ReportRequest createReport(@Valid ReportRequest reportRequest) { | ||
|
|
||
| if (reportRequest.getReportName() == null || reportRequest.getReportName().isEmpty()) { |
There was a problem hiding this comment.
Many important validations are missing, like the query field is not null, and the query has no data modification statement.
| @Table(name = "REPORTREQUEST") | ||
| @Getter | ||
| @Setter | ||
| public class ReportRequest { |
There was a problem hiding this comment.
Can the validation annotations for checks like not null be put as annotations to the relevant fields in the request DTO itself?
| @@ -0,0 +1,64 @@ | |||
| package org.apache.fineract.ReportApi; | |||
There was a problem hiding this comment.
A basic naming convention is to have package names to be in lowercase.
| import javax.validation.Valid; | ||
| import java.util.List; | ||
|
|
||
| @RestController |
There was a problem hiding this comment.
Class level Request Mapping annotation missing.
| @RestController | ||
| public class ReportController { | ||
|
|
||
| @Autowired |
There was a problem hiding this comment.
Annotations to support OAS are missing.
| return ResponseEntity.status(HttpStatus.CREATED).body(createdReport); | ||
| } catch (ValidationException e) { | ||
|
|
||
| return ResponseEntity.badRequest().build(); |
There was a problem hiding this comment.
Too Generic validation handling.
| public ReportRequest updateReport(Long id, ReportRequest updatedReportRequest) { | ||
| Optional<ReportRequest> optionalReport = reportRepository.findById(id); | ||
| if (optionalReport.isPresent()) { | ||
| ReportRequest existingReport = optionalReport.get(); |
There was a problem hiding this comment.
Can every field be updated for a report as per the API specifications?
| existingReport.setReportType(updatedReportRequest.getReportType()); | ||
| existingReport.setReportSubType(updatedReportRequest.getReportSubType()); | ||
| existingReport.setReportCategory(updatedReportRequest.getReportCategory()); | ||
| existingReport.setDescription(updatedReportRequest.getDescription()); |
There was a problem hiding this comment.
Is it necessary to include all field values in the update API request? If so, this design seems inefficient. Only the specific field that requires updating should be included in the request. If not, then why are you setting every other field to null?
The design needs to be corrected here.
2 participants
Description
PHEE-684 Reporting CRUD APIs
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Followed the PR title naming convention mentioned above.
Design related bullet points or design document link related to this PR added in the description above.
Updated corresponding Postman Collection or Api documentation for the changes in this PR.
Create/update unit or integration tests for verifying the changes made.
Add required Swagger annotation and update API documentation with details of any API changes if applicable
Followed the naming conventions as given in https://docs.google.com/document/d/1Q4vaMSzrTxxh9TS0RILuNkSkYCxotuYk1Xe0CMIkkCU/edit?usp=sharing