Skip to content

Add FlowQuery class for GraphQL queries #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
visz11 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add FlowQuery class for GraphQL queries #39

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions FlowQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package com.flecso.employer.graphql.query;

import com.coxautodev.graphql.tools.GraphQLQueryResolver;
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;
Comment on lines +4 to +52
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


import static com.flecso.employer.dto.analytics.Metric.*;

@Component
public class FlowQuery extends BaseQuery implements GraphQLQueryResolver {
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@Autowired
IntegrationService integrationService;
@Autowired
private IssueUnitsRepo issueUnitsRepo;
@Autowired
private TeamService teamService;
@Autowired
private ChartUtilService chartUtilService;
@Autowired
private IssueUnitCustomRepoImpl issueUnitCustomRepo;
@Autowired
private ThresholdService thresholdService;

@Autowired
private GlobalExceptionHandler globalExceptionHandler;
@Autowired
private TeamQuery teamQuery;
@Autowired
private SDLCStageConfigRepo sdlcStageConfigRepo;

ExecutorService executorService = Executors.newFixedThreadPool(8);
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Suggested change
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

Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Suggested change
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

Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Suggested change
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


Gson gson = new Gson();

Logger logger = LoggerFactory.getLogger(FlowQuery.class);
Comment on lines +79 to +83
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.


public Chart getThroughputValueStreamByType(String metricStr, String granularityStr, String breakdown, Filter filter) throws Exception {
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
public Chart getThroughputValueStreamByType(String metricStr, String granularityStr, String breakdown, Filter filter) throws Exception {
public Chart getThroughputValueStreamByType(String metricStr, String granularityStr, Filter filter) throws Exception {

Granularity granularity = StringUtils.isBlank(granularityStr) ? Granularity.WEEK : Granularity.valueOf(granularityStr);
setDateRangeFromSprintId(filter);
List<Map<String, Object>> dataList = new ArrayList<>();
List<Map<String, Object>> prevPeriodDataList = new ArrayList<>();
Pair<PreviousPeriodStat, Double> prevPeriodStatAndTotalPair = Pair.of(PreviousPeriodStat.builder().build(), 0.0);
Threshold threshold = null;
Metric metric = Metric.valueOf(metricStr);
MetricConfig metricConfig = getMetricConfig(metric.name());
ChartMetadata chartMetadata = getChartMetaData(metricConfig);
Chart chart = Chart.builder().chartMetadata(chartMetadata).build();
try {
isIntegrationActive(filter, metricConfig.getSource());
dataList = getValueStreamData(filter, metricConfig, granularity);
double cumulativeOutflow = 0;
double cumulativeInflow = 0;
if (dataList.size() - 1 > 0) {
Copy link

@codeant-ai codeant-ai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Fix off-by-one logic: check for list non-empty with dataList.size() > 0 before accessing last element. [possible bug]

Severity Level: Critical 🚨

Suggested change
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");
Comment on lines +102 to +103
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Suggested change
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

Comment on lines +102 to +103
Copy link

@codeant-ai codeant-ai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Avoid ClassCastException by converting numeric map values via Number.doubleValue() instead of casting Object to int. [possible bug]

Severity Level: Critical 🚨

Suggested change
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.

}
Comment on lines +101 to +104
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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();
}

Comment on lines +101 to +104
Copy link

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

double throughput = 0.0;
if (cumulativeInflow != 0) {
throughput = (cumulativeOutflow*100) / cumulativeInflow;
}
Comment on lines +106 to +108
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Suggested change
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

if (CollectionUtils.isNotEmpty(dataList)) {
switch (metric) {
case ISSUE_THROUGHPUT:
threshold = thresholdService.getThreshold(filter.getOrgId(), filter.getTeamId(), metricConfig, throughput, granularity);
break;
case BUG_THROUGHPUT:
dataList = convertIssueThroughputData(dataList);
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);
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);

Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Suggested change
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

Copy link

@codeant-ai codeant-ai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🚨

Suggested change
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.

} else {
prevPeriodStatAndTotalPair = Pair.of(PreviousPeriodStat.builder().build(), throughput);
}
break;
}
}
} catch (NoIntegrationException noIntegrationException) {
chart.setChartDataState(ChartDataState.NO_INTEGRATION.name());
chart.setChartError(ChartError.builder().message(noIntegrationException.getMessage()).link(noIntegrationException.getLink()).build());
return chart;
} catch (ConfigurationException configurationException) {
chart.setChartDataState(ChartDataState.NOT_CONFIGURED.name());
chart.setChartError(ChartError.builder().message(configurationException.getMessage()).link(configurationException.getLink()).build());
return chart;
} catch (Exception ex) {
globalExceptionHandler.handleGeneralException(ex);
}
Comment on lines +134 to +136
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
        }


return metric.equals(ISSUE_THROUGHPUT) ? chartUtilService.getChartForValueStreamFromProjectData(metricConfig, dataList, threshold, chartMetadata) : chartUtilService.getChartFromProjectData(null, dataList, prevPeriodStatAndTotalPair, chartMetadata, granularity);
Copy link

@codeant-ai codeant-ai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Use enum identity comparison (==) for Metric instead of equals. [enhancement]

Severity Level: Minor ⚠️

Suggested change
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.

}
}