Add FlowQuery class for GraphQL queries #39
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. WalkthroughIntroduces FlowQuery, a new GraphQL query class that computes throughput metrics from value stream data. The getThroughputValueStreamByType method resolves granularities, retrieves data, calculates cumulative inflow/outflow and throughput, applies metric-specific transformations, handles integration exceptions, and returns a Chart via chartUtilService. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant FlowQuery
participant Services as Services<br/>(Autowired)
participant ChartUtil as chartUtilService
Caller->>FlowQuery: getThroughputValueStreamByType(metricStr, granularityStr, breakdown, filter)
rect rgb(220, 240, 250)
Note over FlowQuery,Services: Parse & Resolve
FlowQuery->>FlowQuery: Resolve granularity<br/>from Filter
FlowQuery->>FlowQuery: Compute date range
end
rect rgb(240, 250, 220)
Note over FlowQuery,Services: Data Retrieval & Computation
FlowQuery->>Services: Retrieve value stream data
Services-->>FlowQuery: Value stream data
FlowQuery->>FlowQuery: Compute cumulative<br/>inflow/outflow/throughput
end
rect rgb(250, 240, 220)
Note over FlowQuery: Metric-Specific Logic
alt ISSUE_THROUGHPUT or BUG_THROUGHPUT
FlowQuery->>FlowQuery: Apply special handling<br/>(data conversion,<br/>previous-period stat)
end
end
rect rgb(250, 235, 235)
Note over FlowQuery,Services: Exception Handling
alt NoIntegrationException or<br/>ConfigurationException
FlowQuery->>FlowQuery: Populate Chart<br/>with error state
end
end
rect rgb(230, 240, 250)
Note over FlowQuery,ChartUtil: Chart Generation
FlowQuery->>ChartUtil: Generate Chart<br/>from project data<br/>(metric-specific)
ChartUtil-->>FlowQuery: Final Chart
end
FlowQuery-->>Caller: Return Chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
/refacto-visz |
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 foundational 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
|
Add FlowQuery class for GraphQL queriesTL;DR: Adds new GraphQL query resolver for throughput and value stream analytics with chart generation capabilities. Refacto PR SummaryIntroduces FlowQuery class as a GraphQL resolver for flow-based analytics queries with comprehensive throughput calculations. Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant Client as GraphQL Client
participant FQ as FlowQuery
participant IS as IntegrationService
participant CU as ChartUtilService
participant TS as ThresholdService
participant DB as Database
Client->>FQ: getThroughputValueStreamByType()
FQ->>FQ: Parse metric and granularity
FQ->>IS: isIntegrationActive()
IS-->>FQ: Validation result
FQ->>DB: getValueStreamData()
DB-->>FQ: Raw throughput data
FQ->>FQ: Calculate cumulative flow
FQ->>TS: getThreshold()
TS-->>FQ: Threshold config
FQ->>CU: getChartForValueStreamFromProjectData()
CU-->>FQ: Formatted chart
FQ-->>Client: Chart with metadata
Testing GuideClick to expand
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
codeant-ai
bot
added
the
size:L
![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 introduces a new FlowQuery class to handle GraphQL queries related to flow metrics. The implementation is a good start, but there are several critical areas that need improvement. I've identified a potential resource leak with an unmanaged ExecutorService, an exception handling issue that could lead to unpredictable behavior, and multiple instances of unsafe type casting that could cause runtime exceptions. Additionally, there are some minor code cleanup opportunities, such as removing unused imports, fields, and a method parameter. Addressing these points will significantly improve the robustness and maintainability of the code.
| ExecutorService executorService = Executors.newFixedThreadPool(8); | ||
|
|
||
| Gson gson = new Gson(); | ||
|
|
||
| Logger logger = LoggerFactory.getLogger(FlowQuery.class); |
There was a problem hiding this comment.
The fields executorService, gson, and logger are initialized but never used within this class.
In particular, executorService creates a thread pool that is never shut down, which is a resource leak. If an executor is needed, it should be managed by the Spring container as a bean to handle its lifecycle correctly, and its size should be configurable rather than hardcoded.
If these fields are not needed, they should be removed to clean up the code and prevent resource leaks.
| if (dataList.size() - 1 > 0) { | ||
| cumulativeOutflow = (int) dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | ||
| cumulativeInflow = (int) dataList.get(dataList.size() - 1).get("cumulativeInflow"); | ||
| } |
There was a problem hiding this comment.
The condition dataList.size() - 1 > 0 is unconventional and can be simplified to dataList.size() > 1 for better readability. More importantly, the direct cast from Object to int is unsafe and can cause a ClassCastException or data loss if the value is a Double. It's safer to cast to Number, use getOrDefault to handle missing keys, and then get the appropriate primitive value.
| if (dataList.size() - 1 > 0) { | |
| cumulativeOutflow = (int) dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | |
| cumulativeInflow = (int) dataList.get(dataList.size() - 1).get("cumulativeInflow"); | |
| } | |
| if (dataList.size() > 1) { | |
| Map<String, Object> lastDataPoint = dataList.get(dataList.size() - 1); | |
| cumulativeOutflow = ((Number) lastDataPoint.getOrDefault("cumulativeOutflow", 0)).doubleValue(); | |
| cumulativeInflow = ((Number) lastDataPoint.getOrDefault("cumulativeInflow", 0)).doubleValue(); | |
| } |
| prevPeriodDataList = convertIssueThroughputData(getValueStreamData(filter.lastPeriodFilterInstance(), metricConfig, granularity)); | ||
| throughput = Math.round(throughput * 100.0) / 100.0; | ||
| if (CollectionUtils.isNotEmpty(prevPeriodDataList) && CollectionUtils.isNotEmpty(dataList)) { | ||
| prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat((int) (double) dataList.get(dataList.size() - 1).get("y"), (int) (double) prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y"), metricConfig), throughput); |
There was a problem hiding this comment.
This line is very long and complex, which harms readability and maintainability. It also performs unsafe casts (int) (double) on values retrieved from a map, which can lead to ClassCastException or NullPointerException. The logic should be broken down into smaller, more manageable steps with safe access to map values.
Map<String, Object> lastDataPoint = dataList.get(dataList.size() - 1);
Map<String, Object> lastPrevPeriodDataPoint = prevPeriodDataList.get(prevPeriodDataList.size() - 1);
double currentY = ((Number) lastDataPoint.getOrDefault("y", 0.0)).doubleValue();
double prevPeriodY = ((Number) lastPrevPeriodDataPoint.getOrDefault("y", 0.0)).doubleValue();
prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat((int) currentY, (int) prevPeriodY, metricConfig), throughput);| } catch (Exception ex) { | ||
| globalExceptionHandler.handleGeneralException(ex); | ||
| } |
There was a problem hiding this comment.
The generic catch (Exception ex) block handles the exception but then allows execution to fall through to the final return statement. This can lead to unexpected behavior or further exceptions if the chart data is in an inconsistent state. To ensure predictable behavior, you should return the chart object from within this catch block, similar to how NoIntegrationException and ConfigurationException are handled.
} catch (Exception ex) {
globalExceptionHandler.handleGeneralException(ex);
return chart;
}| import com.flecso.employer.central.entity.GoalTemplate; | ||
| import com.flecso.employer.dto.*; | ||
| import com.flecso.employer.dto.analytics.*; | ||
| import com.flecso.employer.dto.charts.Chart; | ||
| import com.flecso.employer.dto.charts.ChartDataState; | ||
| import com.flecso.employer.dto.charts.ChartError; | ||
| import com.flecso.employer.dto.charts.ChartMetadata; | ||
| import com.flecso.employer.dto.filters.IssueFilterConditions; | ||
| import com.flecso.employer.dto.rechart.ChartKey; | ||
| import com.flecso.employer.entity.IntegrationType; | ||
| import com.flecso.employer.entity.IssueUnits; | ||
| import com.flecso.employer.central.entity.MetricConfig; | ||
| import com.flecso.employer.entity.SDLCStageConfig; | ||
| import com.flecso.employer.exception.ConfigurationException; | ||
| import com.flecso.employer.exception.GlobalExceptionHandler; | ||
| import com.flecso.employer.exception.NoIntegrationException; | ||
| import com.flecso.employer.repository.IssueUnitCustomRepoImpl; | ||
| import com.flecso.employer.repository.IssueUnitsRepo; | ||
| import com.flecso.employer.repository.SDLCStageConfigRepo; | ||
| import com.flecso.employer.service.DateUtil; | ||
| import com.flecso.employer.service.TeamService; | ||
| import com.flecso.employer.service.goal.ThresholdService; | ||
| import com.flecso.employer.service.integration.IntegrationService; | ||
| import com.flecso.employer.util.ChartUtilService; | ||
| import com.flecso.employer.util.ColorUtil; | ||
| import com.flecso.employer.util.Constants; | ||
| import com.flecso.employer.util.CoreUtil; | ||
| import com.github.sisyphsu.dateparser.DateParserUtils; | ||
| import com.google.common.collect.Sets; | ||
| import com.google.gson.Gson; | ||
| import org.apache.commons.collections4.CollectionUtils; | ||
| import org.apache.commons.collections4.MapUtils; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.commons.lang3.tuple.Pair; | ||
| import org.apache.commons.lang3.tuple.Triple; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import java.math.BigDecimal; | ||
| import java.time.LocalDate; | ||
| import java.time.LocalDateTime; | ||
| import java.time.temporal.ChronoUnit; | ||
| import java.util.*; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
| import java.util.stream.Collectors; |
There was a problem hiding this comment.
There are several unused imports in this file. Removing them will improve code clarity and reduce clutter. For example, GoalTemplate, IssueFilterConditions, ChartKey, DateUtil, ColorUtil, Constants, CoreUtil, DateParserUtils, Sets, MapUtils, Triple, BigDecimal, LocalDate, LocalDateTime, ChronoUnit, CompletableFuture, and Collectors appear to be unused. Please run your IDE's 'organize imports' feature to clean them up.
|
|
||
| Logger logger = LoggerFactory.getLogger(FlowQuery.class); | ||
|
|
||
| public Chart getThroughputValueStreamByType(String metricStr, String granularityStr, String breakdown, Filter filter) throws Exception { |
There was a problem hiding this comment.
The method parameter breakdown is not used within the method getThroughputValueStreamByType. It should be removed if it's not needed to avoid confusion and simplify the method signature.
| public Chart getThroughputValueStreamByType(String metricStr, String granularityStr, String breakdown, Filter filter) throws Exception { | |
| public Chart getThroughputValueStreamByType(String metricStr, String granularityStr, Filter filter) throws Exception { |
Code Review: FlowQuery GraphQL Resolver👍 Well Done
📁 Selected files for review (1)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| @Autowired | ||
| private SDLCStageConfigRepo sdlcStageConfigRepo; | ||
|
|
||
| ExecutorService executorService = Executors.newFixedThreadPool(8); |
There was a problem hiding this comment.
Resource Leak Risk
ExecutorService created without proper shutdown mechanism causes thread resource leak. Threads remain active after application shutdown preventing clean resource cleanup. Service restart issues and memory exhaustion result from accumulated thread resources.
@PreDestroy
public void shutdown() {
if (executorService != null && !executorService.isShutdown()) {
executorService.shutdown();
try {
if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) {
executorService.shutdownNow();
}
} catch (InterruptedException e) {
executorService.shutdownNow();
Thread.currentThread().interrupt();
}
}
}
Commitable Suggestion
| ExecutorService executorService = Executors.newFixedThreadPool(8); | |
| @PreDestroy | |
| public void shutdown() { | |
| if (executorService != null && !executorService.isShutdown()) { | |
| executorService.shutdown(); | |
| try { | |
| if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) { | |
| executorService.shutdownNow(); | |
| } | |
| } catch (InterruptedException e) { | |
| executorService.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| } | |
| } | |
| } |
Standards
- ISO-IEC-25010-Reliability-Recoverability
- DbC-Resource-Mgmt
- SRE-Graceful-Shutdown
| cumulativeOutflow = (int) dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | ||
| cumulativeInflow = (int) dataList.get(dataList.size() - 1).get("cumulativeInflow"); |
There was a problem hiding this comment.
Unsafe Type Casting
Unsafe casting from Object to int without type validation creates ClassCastException risk. Map values may contain different numeric types or null values causing runtime failures. Type safety validation required before casting operations.
Object outflowObj = dataList.get(dataList.size() - 1).get("cumulativeOutflow");
Object inflowObj = dataList.get(dataList.size() - 1).get("cumulativeInflow");
cumulativeOutflow = outflowObj instanceof Number ? ((Number) outflowObj).doubleValue() : 0.0;
cumulativeInflow = inflowObj instanceof Number ? ((Number) inflowObj).doubleValue() : 0.0;
Commitable Suggestion
| cumulativeOutflow = (int) dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | |
| cumulativeInflow = (int) dataList.get(dataList.size() - 1).get("cumulativeInflow"); | |
| Object outflowObj = dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | |
| Object inflowObj = dataList.get(dataList.size() - 1).get("cumulativeInflow"); | |
| cumulativeOutflow = outflowObj instanceof Number ? ((Number) outflowObj).doubleValue() : 0.0; | |
| cumulativeInflow = inflowObj instanceof Number ? ((Number) inflowObj).doubleValue() : 0.0; |
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Preconditions
- SRE-Error-Handling
| prevPeriodDataList = convertIssueThroughputData(getValueStreamData(filter.lastPeriodFilterInstance(), metricConfig, granularity)); | ||
| throughput = Math.round(throughput * 100.0) / 100.0; | ||
| if (CollectionUtils.isNotEmpty(prevPeriodDataList) && CollectionUtils.isNotEmpty(dataList)) { | ||
| prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat((int) (double) dataList.get(dataList.size() - 1).get("y"), (int) (double) prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y"), metricConfig), throughput); |
There was a problem hiding this comment.
Complex Unsafe Type Casting
Complex nested type casting without validation creates multiple failure points. Double-to-int casting may lose precision and Object-to-double casting may throw ClassCastException. Chained casting operations increase runtime failure probability significantly.
Object currentYObj = dataList.get(dataList.size() - 1).get("y");
Object prevYObj = prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y");
int currentY = currentYObj instanceof Number ? ((Number) currentYObj).intValue() : 0;
int prevY = prevYObj instanceof Number ? ((Number) prevYObj).intValue() : 0;
prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat(currentY, prevY, metricConfig), throughput);
Commitable Suggestion
| prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat((int) (double) dataList.get(dataList.size() - 1).get("y"), (int) (double) prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y"), metricConfig), throughput); | |
| Object currentYObj = dataList.get(dataList.size() - 1).get("y"); | |
| Object prevYObj = prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y"); | |
| int currentY = currentYObj instanceof Number ? ((Number) currentYObj).intValue() : 0; | |
| int prevY = prevYObj instanceof Number ? ((Number) prevYObj).intValue() : 0; | |
| prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat(currentY, prevY, metricConfig), throughput); |
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Preconditions
- SRE-Error-Handling
| @Autowired | ||
| private SDLCStageConfigRepo sdlcStageConfigRepo; | ||
|
|
||
| ExecutorService executorService = Executors.newFixedThreadPool(8); |
There was a problem hiding this comment.
Hard-coded Thread Pool Configuration
Fixed thread pool size hard-coded without configuration management creates resource exhaustion risk under varying load conditions. Thread pool cannot adapt to system capacity or workload changes causing potential service degradation. Configurable thread pool sizing required for production reliability.
@Value("${app.executor.thread-pool-size:8}")
private int threadPoolSize;
ExecutorService executorService = Executors.newFixedThreadPool(threadPoolSize);
Commitable Suggestion
| ExecutorService executorService = Executors.newFixedThreadPool(8); | |
| @Value("${app.executor.thread-pool-size:8}") | |
| private int threadPoolSize; | |
| ExecutorService executorService = Executors.newFixedThreadPool(threadPoolSize); |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- SRE-Resource-Management
| @Autowired | ||
| private SDLCStageConfigRepo sdlcStageConfigRepo; | ||
|
|
||
| ExecutorService executorService = Executors.newFixedThreadPool(8); |
There was a problem hiding this comment.
Fixed Thread Pool Scalability
Fixed thread pool with hardcoded size creates resource contention under variable load. Thread pool sizing should be configurable and based on system resources. Pool exhaustion causes request queuing and degraded response times.
@Value("${app.executor.core-pool-size:4}")
private int corePoolSize;
@Value("${app.executor.max-pool-size:8}")
private int maxPoolSize;
ExecutorService executorService = new ThreadPoolExecutor(
corePoolSize, maxPoolSize, 60L, TimeUnit.SECONDS,
new LinkedBlockingQueue<>(), new ThreadPoolExecutor.CallerRunsPolicy());
Commitable Suggestion
| ExecutorService executorService = Executors.newFixedThreadPool(8); | |
| @Value("${app.executor.core-pool-size:4}") | |
| private int corePoolSize; | |
| @Value("${app.executor.max-pool-size:8}") | |
| private int maxPoolSize; | |
| ExecutorService executorService = new ThreadPoolExecutor( | |
| corePoolSize, maxPoolSize, 60L, TimeUnit.SECONDS, | |
| new LinkedBlockingQueue<>(), new ThreadPoolExecutor.CallerRunsPolicy()); |
Standards
- ISO-IEC-25010-Performance-Efficiency-Resource-Utilization
- Optimization-Pattern-Thread-Pool-Management
- Algorithmic-Complexity-Resource-Scaling
| if (cumulativeInflow != 0) { | ||
| throughput = (cumulativeOutflow*100) / cumulativeInflow; | ||
| } |
There was a problem hiding this comment.
Division By Zero Validation
Throughput calculation logic has potential precision loss and incomplete validation. Integer casting on line 102-103 may truncate decimal values from Map objects containing doubles. Division operation lacks validation for negative values which could produce misleading throughput percentages in business calculations.
if (cumulativeInflow > 0 && cumulativeOutflow >= 0) {
throughput = (cumulativeOutflow * 100.0) / cumulativeInflow;
} else {
throughput = 0.0;
}
Commitable Suggestion
| if (cumulativeInflow != 0) { | |
| throughput = (cumulativeOutflow*100) / cumulativeInflow; | |
| } | |
| if (cumulativeInflow > 0 && cumulativeOutflow >= 0) { | |
| throughput = (cumulativeOutflow * 100.0) / cumulativeInflow; | |
| } else { | |
| throughput = 0.0; | |
| } |
Standards
- Mathematical-Accuracy-Precision-Preservation
- Business-Rule-Input-Validation
| import static com.flecso.employer.dto.analytics.Metric.*; | ||
|
|
||
| @Component | ||
| public class FlowQuery extends BaseQuery implements GraphQLQueryResolver { |
There was a problem hiding this comment.
God Class Violation
FlowQuery class violates Single Responsibility Principle by handling multiple unrelated concerns including data access, service coordination, exception handling, and chart generation. The class has 10+ dependencies suggesting excessive responsibilities. This creates tight coupling, makes testing difficult, and increases change risk across multiple domains.
Standards
- SOLID-SRP
- Clean-Code-Class-Organization
- Maintainability-Quality-Coupling
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| dataList = getValueStreamData(filter, metricConfig, granularity); | ||
| double cumulativeOutflow = 0; | ||
| double cumulativeInflow = 0; | ||
| if (dataList.size() - 1 > 0) { |
There was a problem hiding this comment.
Suggestion: Fix off-by-one logic: check for list non-empty with dataList.size() > 0 before accessing last element. [possible bug]
Severity Level: Critical 🚨
| if (dataList.size() - 1 > 0) { | |
| if (dataList.size() > 0) { |
Why it matters? ⭐
The current condition (dataList.size() - 1 > 0) is equivalent to dataList.size() > 1, which skips the single-element case but the code then accesses the last element. Changing to dataList.size() > 0 (or !dataList.isEmpty()) correctly allows processing when there's exactly one element and prevents unintended skipping.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** FlowQuery.java
**Line:** 101:101
**Comment:**
*Possible Bug: Fix off-by-one logic: check for list non-empty with dataList.size() > 0 before accessing last element.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| cumulativeOutflow = (int) dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | ||
| cumulativeInflow = (int) dataList.get(dataList.size() - 1).get("cumulativeInflow"); |
There was a problem hiding this comment.
Suggestion: Avoid ClassCastException by converting numeric map values via Number.doubleValue() instead of casting Object to int. [possible bug]
Severity Level: Critical 🚨
| cumulativeOutflow = (int) dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | |
| cumulativeInflow = (int) dataList.get(dataList.size() - 1).get("cumulativeInflow"); | |
| Object outflowObj = dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | |
| Object inflowObj = dataList.get(dataList.size() - 1).get("cumulativeInflow"); | |
| if (outflowObj instanceof Number) { | |
| cumulativeOutflow = ((Number) outflowObj).doubleValue(); | |
| } else if (outflowObj != null) { | |
| cumulativeOutflow = Double.parseDouble(outflowObj.toString()); | |
| } | |
| if (inflowObj instanceof Number) { | |
| cumulativeInflow = ((Number) inflowObj).doubleValue(); | |
| } else if (inflowObj != null) { | |
| cumulativeInflow = Double.parseDouble(inflowObj.toString()); | |
| } |
Why it matters? ⭐
The current code extracts Objects from a Map and casts them with (int) which is brittle and can throw ClassCastException if the stored value is Double, Long, etc. The file contains these exact lines (see getThroughputValueStreamByType) and the variables are declared as double, so using Number.doubleValue() or parsing the toString() is safer and will correctly handle Integer/Double/Long values and avoid runtime casting issues.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** FlowQuery.java
**Line:** 102:103
**Comment:**
*Possible Bug: Avoid ClassCastException by converting numeric map values via Number.doubleValue() instead of casting Object to int.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| prevPeriodDataList = convertIssueThroughputData(getValueStreamData(filter.lastPeriodFilterInstance(), metricConfig, granularity)); | ||
| throughput = Math.round(throughput * 100.0) / 100.0; | ||
| if (CollectionUtils.isNotEmpty(prevPeriodDataList) && CollectionUtils.isNotEmpty(dataList)) { | ||
| prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat((int) (double) dataList.get(dataList.size() - 1).get("y"), (int) (double) prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y"), metricConfig), throughput); |
There was a problem hiding this comment.
Suggestion: Avoid unsafe double/int casts on y values extracted from the maps by converting via Number.intValue()/Integer.parseInt(). [possible bug]
Severity Level: Critical 🚨
| prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat((int) (double) dataList.get(dataList.size() - 1).get("y"), (int) (double) prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y"), metricConfig), throughput); | |
| Object currYObj = dataList.get(dataList.size() - 1).get("y"); | |
| Object prevYObj = prevPeriodDataList.get(prevPeriodDataList.size() - 1).get("y"); | |
| int currY = currYObj instanceof Number ? ((Number) currYObj).intValue() : Integer.parseInt(currYObj.toString()); | |
| int prevY = prevYObj instanceof Number ? ((Number) prevYObj).intValue() : Integer.parseInt(prevYObj.toString()); | |
| prevPeriodStatAndTotalPair = Pair.of(teamQuery.getPreviousPeriodStat(currY, prevY, metricConfig), throughput); |
Why it matters? ⭐
The code currently performs (int)(double) casts on Objects pulled from maps which is fragile (and may fail for Integer/Long/BigDecimal). The same hunk shows these exact casts; converting via Number.intValue() or parsing ensures correct behavior for different numeric types and avoids ClassCastException/Unboxing issues.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** FlowQuery.java
**Line:** 119:119
**Comment:**
*Possible Bug: Avoid unsafe double/int casts on `y` values extracted from the maps by converting via Number.intValue()/Integer.parseInt().
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| globalExceptionHandler.handleGeneralException(ex); | ||
| } | ||
|
|
||
| return metric.equals(ISSUE_THROUGHPUT) ? chartUtilService.getChartForValueStreamFromProjectData(metricConfig, dataList, threshold, chartMetadata) : chartUtilService.getChartFromProjectData(null, dataList, prevPeriodStatAndTotalPair, chartMetadata, granularity); |
There was a problem hiding this comment.
Suggestion: Use enum identity comparison (==) for Metric instead of equals. [enhancement]
Severity Level: Minor
| return metric.equals(ISSUE_THROUGHPUT) ? chartUtilService.getChartForValueStreamFromProjectData(metricConfig, dataList, threshold, chartMetadata) : chartUtilService.getChartFromProjectData(null, dataList, prevPeriodStatAndTotalPair, chartMetadata, granularity); | |
| return metric == ISSUE_THROUGHPUT ? chartUtilService.getChartForValueStreamFromProjectData(metricConfig, dataList, threshold, chartMetadata) : chartUtilService.getChartFromProjectData(null, dataList, prevPeriodStatAndTotalPair, chartMetadata, granularity); |
Why it matters? ⭐
Metric is an enum; comparing enums with == is the recommended and idiomatic approach in Java (null-safety aside if metric can be null). Replacing metric.equals(...) with metric == ISSUE_THROUGHPUT improves clarity and avoids the tiny overhead of an equals call.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** FlowQuery.java
**Line:** 138:138
**Comment:**
*Enhancement: Use enum identity comparison (==) for `Metric` instead of `equals`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if (dataList.size() - 1 > 0) { | ||
| cumulativeOutflow = (int) dataList.get(dataList.size() - 1).get("cumulativeOutflow"); | ||
| cumulativeInflow = (int) dataList.get(dataList.size() - 1).get("cumulativeInflow"); | ||
| } |
There was a problem hiding this comment.
Fix boundary check so single-point data is processed.
When there is exactly one entry in dataList, the guard if (dataList.size() - 1 > 0) skips the body, leaving both cumulative values at 0 and reporting throughput as 0 despite having valid data. This breaks single-bucket charts (e.g., only the latest period), which is a common scenario right after integration or in narrow date ranges. Change the condition to if (dataList.size() > 0) (and similar defensive checks as needed) so that the most recent record is always considered.
🤖 Prompt for AI Agents
In FlowQuery.java around lines 101 to 104, the boundary check skips processing
when dataList has exactly one element; change the guard from `if
(dataList.size() - 1 > 0)` to `if (dataList.size() > 0)` so the last record is
always read, and add minimal defensive checks (verify
dataList.get(dataList.size()-1) is non-null and contains the expected keys or
types before casting) to avoid NPEs or ClassCastExceptions.
CodeAnt-AI Description
Add GraphQL query to return throughput/value-stream charts
What Changed
Impact
✅ View throughput/value-stream charts via GraphQL✅ Previous-period comparison for bug throughput✅ Clearer chart errors when integration missing or metric unconfigured💡 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
Release Notes