Add DataDetailsQuery class for GraphQL queries #38
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new GraphQL query resolver class Changes
Sequence DiagramsequenceDiagram
participant Client as GraphQL Client
participant Resolver as DataDetailsQuery
participant MetricService as MetricService
participant Repo as gitUnitPrCustomRepo
participant Response as Response
Client->>Resolver: getCommitReportDetail(metric, granularity, filter, pageNumber, pageSize)
rect rgb(240, 248, 255)
Note over Resolver: Initialization Phase
Resolver->>Resolver: Initialize date range from filter
Resolver->>MetricService: Resolve MetricConfig by metric name
MetricService-->>Resolver: MetricConfig
Resolver->>Resolver: Convert metric string to Metric enum
Resolver->>Resolver: Derive assignee & project lists from filter & config
Resolver->>Resolver: Build prIds list from filter (if present)
end
rect rgb(230, 245, 230)
Note over Resolver: Data Retrieval Phase
Resolver->>Repo: getCommitReportDetailList(orgId, metricType, dateRange, assignees, repos, prIds, timeZone, pageNumber, pageSize)
Repo-->>Resolver: CommitMetricDetails
end
Resolver->>Response: Return CommitMetricDetails
Response-->>Client: GraphQL Response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new GraphQL query component, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
codeant-ai
bot
added
the
size:M
|
/refacto-visz |
|
CodeAnt AI finished reviewing your PR. |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new DataDetailsQuery class for GraphQL queries. My review focuses on improving maintainability, robustness, and code clarity. The main points include removing a large number of unused dependencies, adopting constructor injection over field injection, improving error handling, and other minor code refinements. Addressing these points will make the code cleaner and more robust.
| @Autowired | ||
| IssueUnitCustomRepoImpl issueUnitCustomRepo; | ||
| @Autowired | ||
| BuildDeploymentUnitCustomRepo buildDeploymentUnitCustomRepo; | ||
|
|
||
| @Autowired | ||
| GitUnitPrCustomRepoImpl gitUnitPrCustomRepo; | ||
|
|
||
| @Autowired | ||
| VulnerabilityUnitsCustomRepo vulnerabilityUnitsCustomRepo; | ||
|
|
||
| @Autowired | ||
| EventLogGitCustomRepoImpl eventLogGitCustomRepo; | ||
|
|
||
| @Autowired | ||
| ContributorService contributorService; | ||
|
|
||
| @Autowired | ||
| Games24X7CustomRepoImpl games24X7CustomRepo; | ||
|
|
||
| @Autowired | ||
| TeamService teamService; | ||
| @Autowired | ||
| private CustomMetricService customMetricService; | ||
| @Autowired | ||
| private GlobalExceptionHandler globalExceptionHandler; | ||
|
|
||
| @Autowired | ||
| private WorkBreakdownCustomRepo workBreakdownCustomRepo; | ||
| @Autowired | ||
| private MetricsMetadataService metricsMetadataService; | ||
| @Autowired | ||
| private RecruitCRMCustomQueryRepoImpl recruitCRMCustomQueryRepo; | ||
| @Autowired | ||
| private DeliverableService deliverableService; | ||
| @Autowired | ||
| private GitMetricCalculationStrategy gitMetricCalculationStrategy; | ||
| @Autowired | ||
| private SprintCustomRepoImpl sprintCustomRepo; |
There was a problem hiding this comment.
This class injects a large number of dependencies, but only gitUnitPrCustomRepo is actually used. This violates the YAGNI (You Ain't Gonna Need It) principle and makes the class harder to maintain. Additionally, it's a best practice to use constructor injection instead of field injection. Constructor injection makes dependencies explicit, improves testability, and helps ensure objects are always in a valid state. Please remove all unused dependencies and use constructor injection for the required ones.
| @Autowired | |
| IssueUnitCustomRepoImpl issueUnitCustomRepo; | |
| @Autowired | |
| BuildDeploymentUnitCustomRepo buildDeploymentUnitCustomRepo; | |
| @Autowired | |
| GitUnitPrCustomRepoImpl gitUnitPrCustomRepo; | |
| @Autowired | |
| VulnerabilityUnitsCustomRepo vulnerabilityUnitsCustomRepo; | |
| @Autowired | |
| EventLogGitCustomRepoImpl eventLogGitCustomRepo; | |
| @Autowired | |
| ContributorService contributorService; | |
| @Autowired | |
| Games24X7CustomRepoImpl games24X7CustomRepo; | |
| @Autowired | |
| TeamService teamService; | |
| @Autowired | |
| private CustomMetricService customMetricService; | |
| @Autowired | |
| private GlobalExceptionHandler globalExceptionHandler; | |
| @Autowired | |
| private WorkBreakdownCustomRepo workBreakdownCustomRepo; | |
| @Autowired | |
| private MetricsMetadataService metricsMetadataService; | |
| @Autowired | |
| private RecruitCRMCustomQueryRepoImpl recruitCRMCustomQueryRepo; | |
| @Autowired | |
| private DeliverableService deliverableService; | |
| @Autowired | |
| private GitMetricCalculationStrategy gitMetricCalculationStrategy; | |
| @Autowired | |
| private SprintCustomRepoImpl sprintCustomRepo; | |
| private final GitUnitPrCustomRepoImpl gitUnitPrCustomRepo; | |
| @Autowired | |
| public DataDetailsQuery(GitUnitPrCustomRepoImpl gitUnitPrCustomRepo) { | |
| this.gitUnitPrCustomRepo = gitUnitPrCustomRepo; | |
| } |
| @Autowired | ||
| private SprintCustomRepoImpl sprintCustomRepo; | ||
|
|
||
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { |
There was a problem hiding this comment.
The granularity parameter is not used within the getCommitReportDetail method. It should be removed to simplify the method signature and avoid confusion.
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | |
| public CommitMetricDetails getCommitReportDetail(String metric, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { |
| @Autowired | ||
| private SprintCustomRepoImpl sprintCustomRepo; | ||
|
|
||
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { |
There was a problem hiding this comment.
The method signature declares throws Exception, which is too broad and can hide specific issues. It's better to declare the specific checked exceptions that the method can throw, or handle them inside the method. This makes error handling more robust and explicit for the callers of this method.
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | ||
| setDateRangeFromSprintId(filter); | ||
| MetricConfig metricConfig = getMetricConfig(metric); | ||
| Metric metricType = Metric.valueOf(metric); |
There was a problem hiding this comment.
Metric.valueOf(metric) will throw an IllegalArgumentException if the input metric string does not match any enum constant. This is an unchecked exception that will result in a server error if not handled. It's better to validate the input or catch the exception to provide a more graceful error response to the client, for example, a GraphQL error indicating a bad request.
Example:
try {
Metric metricType = Metric.valueOf(metric);
} catch (IllegalArgumentException e) {
// It's good practice to throw a specific GraphQL exception
// that will be translated into a proper error response.
throw new GraphQLException("Invalid 'metric' parameter provided.");
}| Pair<List<String>, List<String>> assigneeAndProjectList = getAssigneeAndProjectList(filter, metricConfig); | ||
| List<String> repoList = assigneeAndProjectList.getRight(); | ||
| List<String> assignees = assigneeAndProjectList.getLeft(); | ||
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : new ArrayList<>(); |
There was a problem hiding this comment.
This line can be slightly improved. Using Collections.emptyList() is generally preferred over new ArrayList<>() for creating an empty list, as it avoids allocating a new object when no PR IDs are provided.
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : new ArrayList<>(); | |
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : Collections.emptyList(); |
| List<String> assignees = assigneeAndProjectList.getLeft(); | ||
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : new ArrayList<>(); | ||
|
|
||
| CommitMetricDetails commitMetricDetails = gitUnitPrCustomRepo.getCommitReportDetailList(filter.getOrgId(), metricType, filter.getLocalStartDateFromEndDate(), filter.getEndDate(),assignees, repoList, prIds, filter.getTimeZone(), pageNumber, pageSize); |
There was a problem hiding this comment.
The method getCommitReportDetailList is assigned to a variable of type CommitMetricDetails, which seems to be a single object, not a list. The List suffix in the method name is misleading. If the method returns a single object, consider renaming it in GitUnitPrCustomRepoImpl to something like getCommitReportDetails to improve clarity.
Code Review: GraphQL Query Implementation👍 Well Done
📁 Selected files for review (1)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| List<String> assignees = assigneeAndProjectList.getLeft(); | ||
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : new ArrayList<>(); | ||
|
|
||
| CommitMetricDetails commitMetricDetails = gitUnitPrCustomRepo.getCommitReportDetailList(filter.getOrgId(), metricType, filter.getLocalStartDateFromEndDate(), filter.getEndDate(),assignees, repoList, prIds, filter.getTimeZone(), pageNumber, pageSize); |
There was a problem hiding this comment.
SQL Injection Risk
Multiple user-controlled parameters passed to database query method without visible input validation. Filter parameters like orgId, date ranges, assignees, and prIds could contain malicious payloads leading to SQL injection if not properly sanitized in the repository layer.
Standards
- CWE-89
- OWASP-A03
- Org-Guideline-Validate all external inputs
| @Autowired | ||
| private SprintCustomRepoImpl sprintCustomRepo; | ||
|
|
||
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { |
There was a problem hiding this comment.
Missing Input Validation
GraphQL query method accepts user input parameters without validation checks. Missing validation on metric name, pagination parameters, and filter objects could lead to injection attacks or application errors.
Standards
- CWE-20
- OWASP-A03
- Org-Guideline-Validate all external inputs
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | ||
| setDateRangeFromSprintId(filter); | ||
| MetricConfig metricConfig = getMetricConfig(metric); | ||
| Metric metricType = Metric.valueOf(metric); |
There was a problem hiding this comment.
Null Parameter Risk
Method parameters used without null validation before processing operations. Null filter parameter passed to setDateRangeFromSprintId() or null metric parameter to Metric.valueOf() causes NullPointerException.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- DbC-Preconditions
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | ||
| setDateRangeFromSprintId(filter); | ||
| MetricConfig metricConfig = getMetricConfig(metric); | ||
| Metric metricType = Metric.valueOf(metric); |
There was a problem hiding this comment.
Enum Conversion Failure
Metric.valueOf() throws IllegalArgumentException for invalid metric string values without exception handling. Invalid metric parameter causes uncaught runtime exception terminating GraphQL query processing.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| import java.util.stream.Collectors; | ||
|
|
||
| @Component | ||
| public class DataDetailsQuery extends BaseQuery implements GraphQLQueryResolver { |
There was a problem hiding this comment.
God Object Pattern
Class violates Single Responsibility Principle with 14 injected dependencies handling diverse concerns (issues, builds, vulnerabilities, git, contributors, games, teams, metrics, exceptions, work breakdown, recruitment, deliverables, sprints). This creates excessive coupling and makes the class fragile to changes across multiple domains.
@Component
public class DataDetailsQuery extends BaseQuery implements GraphQLQueryResolver {
@Autowired
private CommitReportService commitReportService;
public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception {
return commitReportService.getCommitReportDetail(metric, granularity, filter, pageNumber, pageSize);
}
}
Commitable Suggestion
| public class DataDetailsQuery extends BaseQuery implements GraphQLQueryResolver { | |
| @Component | |
| public class DataDetailsQuery extends BaseQuery implements GraphQLQueryResolver { | |
| @Autowired | |
| private CommitReportService commitReportService; | |
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | |
| return commitReportService.getCommitReportDetail(metric, granularity, filter, pageNumber, pageSize); | |
| } | |
| } |
Standards
- SOLID-SRP
- Clean-Code-Class-Organization
- Maintainability-Quality-Coupling
| @Autowired | ||
| IssueUnitCustomRepoImpl issueUnitCustomRepo; | ||
| @Autowired | ||
| BuildDeploymentUnitCustomRepo buildDeploymentUnitCustomRepo; | ||
|
|
||
| @Autowired | ||
| GitUnitPrCustomRepoImpl gitUnitPrCustomRepo; |
There was a problem hiding this comment.
Field Injection Pattern
Field injection creates dependencies that are difficult to test and may cause initialization order issues. Constructor injection provides better testability, immutability, and fail-fast behavior during application startup.
Standards
- ISO-IEC-25010-Performance-Efficiency-Resource-Utilization
- Dependency-Injection-Best-Practices
| @Autowired | ||
| IssueUnitCustomRepoImpl issueUnitCustomRepo; | ||
| @Autowired |
There was a problem hiding this comment.
Inconsistent Field Visibility
Inconsistent field visibility mixing package-private and private access modifiers for dependency injection. Some fields lack private modifier while others include it, creating unclear encapsulation boundaries.
Standards
- Clean-Code-Encapsulation
- SOLID-OCP
- Maintainability-Quality-Consistency
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
DataDetailsQuery.java (1)
40-78: Consider using constructor injection instead of field injection.Field injection via
@Autowiredmakes the class harder to test and hides dependencies. Constructor injection is the preferred approach for better testability and explicit dependency declaration.Example refactor (showing pattern for a few dependencies):
- @Autowired - GitUnitPrCustomRepoImpl gitUnitPrCustomRepo; - - @Autowired - private GlobalExceptionHandler globalExceptionHandler; + private final GitUnitPrCustomRepoImpl gitUnitPrCustomRepo; + private final GlobalExceptionHandler globalExceptionHandler; + + @Autowired + public DataDetailsQuery( + GitUnitPrCustomRepoImpl gitUnitPrCustomRepo, + GlobalExceptionHandler globalExceptionHandler + ) { + this.gitUnitPrCustomRepo = gitUnitPrCustomRepo; + this.globalExceptionHandler = globalExceptionHandler; + }
| @Autowired | ||
| IssueUnitCustomRepoImpl issueUnitCustomRepo; | ||
| @Autowired | ||
| BuildDeploymentUnitCustomRepo buildDeploymentUnitCustomRepo; | ||
|
|
||
| @Autowired | ||
| GitUnitPrCustomRepoImpl gitUnitPrCustomRepo; | ||
|
|
||
| @Autowired | ||
| VulnerabilityUnitsCustomRepo vulnerabilityUnitsCustomRepo; | ||
|
|
||
| @Autowired | ||
| EventLogGitCustomRepoImpl eventLogGitCustomRepo; | ||
|
|
||
| @Autowired | ||
| ContributorService contributorService; | ||
|
|
||
| @Autowired | ||
| Games24X7CustomRepoImpl games24X7CustomRepo; | ||
|
|
||
| @Autowired | ||
| TeamService teamService; | ||
| @Autowired | ||
| private CustomMetricService customMetricService; | ||
| @Autowired | ||
| private GlobalExceptionHandler globalExceptionHandler; | ||
|
|
||
| @Autowired | ||
| private WorkBreakdownCustomRepo workBreakdownCustomRepo; | ||
| @Autowired | ||
| private MetricsMetadataService metricsMetadataService; | ||
| @Autowired | ||
| private RecruitCRMCustomQueryRepoImpl recruitCRMCustomQueryRepo; | ||
| @Autowired | ||
| private DeliverableService deliverableService; | ||
| @Autowired | ||
| private GitMetricCalculationStrategy gitMetricCalculationStrategy; | ||
| @Autowired | ||
| private SprintCustomRepoImpl sprintCustomRepo; |
There was a problem hiding this comment.
🧩 Analysis chain
Remove unused autowired dependencies.
Most of the 16 autowired dependencies appear unused in this class. Only gitUnitPrCustomRepo is directly referenced in the getCommitReportDetail method. The others (issueUnitCustomRepo, buildDeploymentUnitCustomRepo, vulnerabilityUnitsCustomRepo, eventLogGitCustomRepo, contributorService, games24X7CustomRepo, teamService, customMetricService, globalExceptionHandler, workBreakdownCustomRepo, metricsMetadataService, recruitCRMCustomQueryRepo, deliverableService, gitMetricCalculationStrategy, sprintCustomRepo) should be removed unless they're used by inherited methods in BaseQuery.
Run the following script to verify which dependencies are actually used:
🏁 Script executed:
#!/bin/bash
# Description: Check if each autowired dependency is referenced in DataDetailsQuery.java
echo "Checking dependency usage in DataDetailsQuery.java..."
for dep in "issueUnitCustomRepo" "buildDeploymentUnitCustomRepo" "vulnerabilityUnitsCustomRepo" \
"eventLogGitCustomRepo" "contributorService" "games24X7CustomRepo" \
"teamService" "customMetricService" "globalExceptionHandler" \
"workBreakdownCustomRepo" "metricsMetadataService" "recruitCRMCustomQueryRepo" \
"deliverableService" "gitMetricCalculationStrategy" "sprintCustomRepo"; do
echo "---"
echo "Dependency: $dep"
rg -n "\b$dep\b" --type java DataDetailsQuery.java | grep -v "@Autowired" || echo " NOT USED in DataDetailsQuery.java"
doneLength of output: 1496
The web search provides context about Spring GraphQL architecture but not specifics about this codebase. Let me generate shell scripts to verify the actual usage of these dependencies in the repository.
🏁 Script executed:
#!/bin/bash
# Find DataDetailsQuery.java and display full content
echo "=== Finding DataDetailsQuery.java ==="
fd -e java -i "datadetailsquery" --exec cat {} \;Length of output: 4016
🏁 Script executed:
#!/bin/bash
# Find BaseQuery.java
echo "=== Finding BaseQuery.java ==="
fd -e java -i "basequery" --exec cat {} \;Length of output: 30
🏁 Script executed:
#!/bin/bash
# Search for usages of gitUnitPrCustomRepo across the codebase
echo "=== Checking gitUnitPrCustomRepo usage in full repository ==="
rg -n "gitUnitPrCustomRepo" --type java -C2 | head -50Length of output: 888
🏁 Script executed:
#!/bin/bash
# Check if any other dependencies are used in the entire codebase
echo "=== Checking if unused dependencies are referenced elsewhere in codebase ==="
for dep in "issueUnitCustomRepo" "buildDeploymentUnitCustomRepo" "vulnerabilityUnitsCustomRepo" \
"eventLogGitCustomRepo" "contributorService" "games24X7CustomRepo" \
"teamService" "customMetricService" "globalExceptionHandler" \
"workBreakdownCustomRepo" "metricsMetadataService" "recruitCRMCustomQueryRepo" \
"deliverableService" "gitMetricCalculationStrategy" "sprintCustomRepo"; do
count=$(rg -c "\b$dep\b" --type java 2>/dev/null | awk -F: '{sum+=$2} END {print sum}')
if [ "$count" -gt 1 ]; then
echo "$dep: used $count times (likely in multiple files)"
fi
doneLength of output: 76
Remove unused autowired dependencies from DataDetailsQuery.java (lines 40-78).
Verification confirms 14 of the 15 autowired dependencies are completely unused in this class. The getCommitReportDetail() method only uses gitUnitPrCustomRepo directly, and delegates to inherited BaseQuery methods for other logic. Remove the following unused dependencies:
issueUnitCustomRepo(line 41)buildDeploymentUnitCustomRepo(line 43)vulnerabilityUnitsCustomRepo(line 49)eventLogGitCustomRepo(line 52)contributorService(line 55)games24X7CustomRepo(line 58)teamService(line 61)customMetricService(line 63)globalExceptionHandler(line 65)workBreakdownCustomRepo(line 68)metricsMetadataService(line 70)recruitCRMCustomQueryRepo(line 72)deliverableService(line 74)gitMetricCalculationStrategy(line 76)sprintCustomRepo(line 78)
Retain only gitUnitPrCustomRepo (line 46), which is actively used in the method body at line 89.
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | ||
| setDateRangeFromSprintId(filter); | ||
| MetricConfig metricConfig = getMetricConfig(metric); | ||
| Metric metricType = Metric.valueOf(metric); | ||
| Pair<List<String>, List<String>> assigneeAndProjectList = getAssigneeAndProjectList(filter, metricConfig); | ||
| List<String> repoList = assigneeAndProjectList.getRight(); | ||
| List<String> assignees = assigneeAndProjectList.getLeft(); | ||
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : new ArrayList<>(); | ||
|
|
||
| CommitMetricDetails commitMetricDetails = gitUnitPrCustomRepo.getCommitReportDetailList(filter.getOrgId(), metricType, filter.getLocalStartDateFromEndDate(), filter.getEndDate(),assignees, repoList, prIds, filter.getTimeZone(), pageNumber, pageSize); | ||
| return commitMetricDetails; | ||
| } |
There was a problem hiding this comment.
Add input validation for method parameters.
The method lacks validation for its parameters, which can lead to runtime exceptions and security issues:
- No null check on
filter- Line 81 will throw NPE if filter is null - No validation on
pageNumberandpageSize- Negative, zero, or excessively large values could cause issues - No validation on
metric- Line 83'sMetric.valueOf(metric)will throwIllegalArgumentExceptionfor invalid values
Add validation at the beginning of the method:
public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception {
+ if (filter == null) {
+ throw new IllegalArgumentException("Filter cannot be null");
+ }
+ if (StringUtils.isBlank(metric)) {
+ throw new IllegalArgumentException("Metric cannot be null or empty");
+ }
+ if (pageNumber == null || pageNumber < 1) {
+ throw new IllegalArgumentException("Page number must be greater than 0");
+ }
+ if (pageSize == null || pageSize < 1 || pageSize > 1000) {
+ throw new IllegalArgumentException("Page size must be between 1 and 1000");
+ }
+
setDateRangeFromSprintId(filter);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | |
| setDateRangeFromSprintId(filter); | |
| MetricConfig metricConfig = getMetricConfig(metric); | |
| Metric metricType = Metric.valueOf(metric); | |
| Pair<List<String>, List<String>> assigneeAndProjectList = getAssigneeAndProjectList(filter, metricConfig); | |
| List<String> repoList = assigneeAndProjectList.getRight(); | |
| List<String> assignees = assigneeAndProjectList.getLeft(); | |
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : new ArrayList<>(); | |
| CommitMetricDetails commitMetricDetails = gitUnitPrCustomRepo.getCommitReportDetailList(filter.getOrgId(), metricType, filter.getLocalStartDateFromEndDate(), filter.getEndDate(),assignees, repoList, prIds, filter.getTimeZone(), pageNumber, pageSize); | |
| return commitMetricDetails; | |
| } | |
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | |
| if (filter == null) { | |
| throw new IllegalArgumentException("Filter cannot be null"); | |
| } | |
| if (StringUtils.isBlank(metric)) { | |
| throw new IllegalArgumentException("Metric cannot be null or empty"); | |
| } | |
| if (pageNumber == null || pageNumber < 1) { | |
| throw new IllegalArgumentException("Page number must be greater than 0"); | |
| } | |
| if (pageSize == null || pageSize < 1 || pageSize > 1000) { | |
| throw new IllegalArgumentException("Page size must be between 1 and 1000"); | |
| } | |
| setDateRangeFromSprintId(filter); | |
| MetricConfig metricConfig = getMetricConfig(metric); | |
| Metric metricType = Metric.valueOf(metric); | |
| Pair<List<String>, List<String>> assigneeAndProjectList = getAssigneeAndProjectList(filter, metricConfig); | |
| List<String> repoList = assigneeAndProjectList.getRight(); | |
| List<String> assignees = assigneeAndProjectList.getLeft(); | |
| List<String> prIds = filter.getPrViewFilters() != null && CollectionUtils.isNotEmpty(filter.getPrViewFilters().getPrIds()) ? filter.getPrViewFilters().getPrIds() : new ArrayList<>(); | |
| CommitMetricDetails commitMetricDetails = gitUnitPrCustomRepo.getCommitReportDetailList(filter.getOrgId(), metricType, filter.getLocalStartDateFromEndDate(), filter.getEndDate(),assignees, repoList, prIds, filter.getTimeZone(), pageNumber, pageSize); | |
| return commitMetricDetails; | |
| } |
🤖 Prompt for AI Agents
In DataDetailsQuery.java around lines 80 to 91, the method getCommitReportDetail
lacks input validation: add an explicit null check for filter and throw
IllegalArgumentException with a clear message if null; validate metric is
non-null/non-empty and safely parse it (catch IllegalArgumentException from
Metric.valueOf and rethrow a descriptive IllegalArgumentException); validate
pageNumber and pageSize (non-null, >0, and enforce a sensible max like 1000 or
use defaults) and throw IllegalArgumentException for invalid values; after
validation proceed to call setDateRangeFromSprintId and the rest so downstream
calls cannot cause NPEs or unexpected exceptions.
| @Autowired | ||
| private SprintCustomRepoImpl sprintCustomRepo; | ||
|
|
||
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { |
There was a problem hiding this comment.
Fix unused parameter and overly broad exception declaration.
Two critical issues with the method signature:
- The
granularityparameter is never used in the method body, suggesting either incomplete implementation or that it should be removed. - Declaring
throws Exceptionis too broad and considered an anti-pattern. Throw specific exceptions (e.g.,ConfigurationException) or let Spring's exception handling deal with unchecked exceptions.
Apply this diff to remove the unused parameter and narrow the exception type:
-public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception {
+public CommitMetricDetails getCommitReportDetail(String metric, Filter filter, Integer pageNumber, Integer pageSize) throws ConfigurationException {If granularity is actually needed, please implement its usage in the method body.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In DataDetailsQuery.java around line 80, the method signature declares an unused
parameter 'granularity' and broadly throws Exception; remove the unused
'granularity' parameter from the method signature (or if it is required,
implement its logic in the method body where metrics are computed/filtered) and
replace the broad throws Exception with specific checked exceptions (e.g.,
ConfigurationException, IOException) or remove the throws clause entirely and
let unchecked exceptions propagate so the controller/Spring exception handler
handles them; update any callers to match the new signature.
| public CommitMetricDetails getCommitReportDetail(String metric, String granularity, Filter filter, Integer pageNumber, Integer pageSize) throws Exception { | ||
| setDateRangeFromSprintId(filter); | ||
| MetricConfig metricConfig = getMetricConfig(metric); | ||
| Metric metricType = Metric.valueOf(metric); |
There was a problem hiding this comment.
Add error handling for enum conversion.
The Metric.valueOf(metric) call will throw an IllegalArgumentException if the metric string doesn't match any enum constant. This could expose internal implementation details to GraphQL clients.
Wrap the conversion with proper error handling:
- Metric metricType = Metric.valueOf(metric);
+ Metric metricType;
+ try {
+ metricType = Metric.valueOf(metric);
+ } catch (IllegalArgumentException e) {
+ throw new ConfigurationException(ErrorCode.INVALID_METRIC, "Invalid metric: " + metric);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In DataDetailsQuery.java around line 83, Metric.valueOf(metric) can throw
IllegalArgumentException for invalid input; wrap the conversion in a try-catch
that catches IllegalArgumentException, log the invalid metric (without sensitive
internals), and rethrow a safe GraphQL-friendly exception (e.g. new
IllegalArgumentException or a DataFetchingException/GraphQLException with a
sanitized message like "Invalid metric specified") so callers get a clear,
non-sensitive error; optionally consider using a case-insensitive lookup or
EnumUtils to normalize before conversion.
CodeAnt-AI Description
Expose commit report details via GraphQL with filters and pagination
What Changed
Impact
✅ Retrieve commit reports via GraphQL✅ Paginated commit detail responses✅ Filter commits by PR IDs and sprint date range💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit