updated for elastic v8 - #415
Conversation
WalkthroughElasticsearch TransportClient dependencies and code paths are removed. Repositories are refactored to use ESHttpClient and ESQueryFactory over HTTP with new index constants and sort validation. The ElasticSearchQueryFactory class is deleted. Application bootstrapping drops ES client setup and updates WebMvcConfigurer usage. A controller adjusts ResponseInfo timestamp and resMsgId handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as InstrumentAccountCodeController
participant Repo as *ESRepository (Instrument/Type/AccountCode/SurrenderReason)*
participant ESQ as ESQueryFactory
participant HTTP as ESHttpClient
participant ES as Elasticsearch
Client->>Controller: search request
Controller->>Repo: search(criteria)
Repo->>ESQ: buildQuery(criteria)
ESQ-->>Repo: Map query
Repo->>HTTP: search(index, query, DomainClass)
HTTP->>ES: HTTP request (query DSL)
ES-->>HTTP: hits + pagination
HTTP-->>Repo: Pagination<Domain>
Repo-->>Controller: Pagination<Domain>
Controller-->>Client: response (ResponseInfo with ts in ms)
alt Error
Repo->>Repo: catch Exception
Repo-->>Controller: empty Pagination
Controller-->>Client: response with empty results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
financial-module-system/egf-instrument/pom.xml (3)
68-71: Outdated log4j-to-slf4j; align or remove.2.8.2 is very old and mismatches log4j2.version (2.17.1). Either remove this bridge (Logback is default) or bump to ≥2.17.2 and align via a property.
Apply (if keeping):
- <dependency> - <groupId>org.apache.logging.log4j</groupId> - <artifactId>log4j-to-slf4j</artifactId> - <version>2.8.2</version> - </dependency> + <dependency> + <groupId>org.apache.logging.log4j</groupId> + <artifactId>log4j-to-slf4j</artifactId> + <version>2.17.2</version> + </dependency>
73-76: commons-lang3 3.0 is a decade old; upgrade.Upgrade to a recent 3.x to pick up fixes and JDK 17 improvements.
Example:
- <dependency> - <groupId>org.apache.commons</groupId> - <artifactId>commons-lang3</artifactId> - <version>3.0</version> - </dependency> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-lang3</artifactId> + <version>3.14.0</version> + </dependency>
142-145: JaCoCo 0.7.9 is incompatible with Java 17+; update the plugin.This version will misreport or fail under JDK 17. Move to ≥0.8.8.
Apply:
- <artifactId>jacoco-maven-plugin</artifactId> - <version>0.7.9</version> + <artifactId>jacoco-maven-plugin</artifactId> + <version>0.8.11</version>financial-module-system/egf-instrument/src/main/java/org/egov/EgfInstrumentApplication.java (1)
57-60: MediaType.APPLICATION_JSON_UTF8 was removed in Spring 6; use APPLICATION_JSON.This will not compile on Boot 3.x/Spring 6.x.
Apply:
- configurer.defaultContentType(MediaType.APPLICATION_JSON_UTF8); + configurer.defaultContentType(MediaType.APPLICATION_JSON);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
financial-module-system/egf-instrument/pom.xml(1 hunks)financial-module-system/egf-instrument/src/main/java/org/egov/EgfInstrumentApplication.java(3 hunks)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/ElasticSearchQueryFactory.java(0 hunks)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java(1 hunks)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentESRepository.java(1 hunks)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentTypeESRepository.java(1 hunks)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java(1 hunks)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/web/controller/InstrumentAccountCodeController.java(1 hunks)
💤 Files with no reviewable changes (1)
- financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/ElasticSearchQueryFactory.java
🧰 Additional context used
🧬 Code graph analysis (5)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/web/controller/InstrumentAccountCodeController.java (1)
frontend/micro-ui/web/micro-ui-internals/packages/libraries/src/services/atoms/Utils/Request.js (1)
ts(73-73)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java (3)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java (1)
Service(15-49)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentESRepository.java (1)
Service(15-49)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentTypeESRepository.java (1)
Service(16-51)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java (3)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentESRepository.java (1)
Service(15-49)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentTypeESRepository.java (1)
Service(16-51)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java (1)
Service(15-50)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentTypeESRepository.java (3)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java (1)
Service(15-49)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentESRepository.java (1)
Service(15-49)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java (1)
Service(15-50)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentESRepository.java (3)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java (1)
Service(15-49)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentTypeESRepository.java (1)
Service(16-51)financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java (1)
Service(15-50)
🔇 Additional comments (14)
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java (2)
27-36: Good: query built via factory and delegated to HTTP clientClean separation of concerns; consistent with other repositories.
3-13: Add missing imports for ESHttpClient and ESQueryFactory
SurrenderReasonESRepository.java references ESHttpClient and ESQueryFactory without importing their packages; add the correct FQCN imports (e.g.org.egov.common.persistence.client.ESHttpClientandorg.egov.common.persistence.query.ESQueryFactoryor wherever they’re defined) to prevent compilation failures.financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/web/controller/InstrumentAccountCodeController.java (1)
178-179: Epoch millis for ts is correct and consistent with micro-ui.Matches frontend usage (Date.getTime()). LGTM.
financial-module-system/egf-instrument/src/main/java/org/egov/EgfInstrumentApplication.java (2)
16-16: Migration to WebMvcConfigurer is correct.Good modernization for Spring 6/Boot 3.
36-38: init() cleanup looks good.Dropping checked exceptions and only setting timezone is appropriate.
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java (4)
33-33: Avoid logging PII in query debug logs.Confirm the query map never contains sensitive fields (account numbers, user identifiers). If present, sanitize before logging.
18-36: Migration aligns with v8 and shared pattern across repositories.Consistent use of ESHttpClient/ESQueryFactory and centralized validation is good.
43-47: Validate sortBy against ES index fields rather than JPA entity fields. The call to validateEntityFieldName(sortBy, InstrumentAccountCodeEntity.class) only checks the Java entity’s declared fields—if your ES mapping uses different names (nested paths, snake_case, etc.), valid sort parameters will be rejected. Consider switching to validation against the ES search contract or an explicit whitelist of sortable ES fields.
31-36: Verify ESQueryFactory pagination fields
Cannot find theESQueryFactoryimplementation in this module—please confirm thatbuildInstrumentAccountCodeQuerysetstrack_total_hits(for ES v8), correctly maps pagination (from/sizeorsearch_after), and applies sorting to ensure accurate pagination totals.financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentESRepository.java (3)
33-33: Scrub debug logs for sensitive data.Ensure the query map excludes PII; otherwise, mask before logging.
18-36: Overall migration looks consistent and clean.Adopts the shared ESHttpClient/ESQueryFactory pattern and centralizes sort validation. Good step towards ES v8.
31-36: Ensure ESQueryFactory.buildInstrumentQuery produces ES 8-compatible queries
No evidence was found oftrack_total_hits,search_after, or explicitfrom/sizeparameters in the query map. Manually verify thatbuildInstrumentQueryattaches pagination (or usessearch_afterfor deep paging), sort clauses, andtrack_total_hitsfor total counts.financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentTypeESRepository.java (2)
32-37: ES v8 HTTP migration flow LGTMUsing ESQueryFactory to build the query and ESHttpClient.search(...) is consistent with other repositories and keeps concerns separated.
28-41: Declare non-null contract, refine error handling, verify ES pagination
- At the start of
search, add
Objects.requireNonNull(instrumentTypeSearchContract, "instrumentTypeSearchContract must not be null");and importjava.util.Objects.- Replace the broad
catch (Exception e)with more specific exception handling or rethrow a domain exception to avoid silently swallowing errors.- Confirm that
ESHttpClient.searchsets ES’strack_total_hits(for ES 7/8) so pagination’s total count is accurate.
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Rely on Spring Boot’s managed Jackson unless you have a direct need.
spring-boot-starter-web already brings jackson-databind. Keeping an explicit dependency is fine, but consider removing it to avoid version drift.
🤖 Prompt for AI Agents
In financial-module-system/egf-instrument/pom.xml around lines 59 to 61, an
explicit jackson-databind dependency is declared even though
spring-boot-starter-web already manages Jackson; remove this explicit dependency
to avoid version drift, or if you truly need to control the Jackson version,
move the version into the parent BOM/dependencyManagement (or align it with the
Spring Boot managed version) and document why the override is required.
| <groupId>jakarta.json</groupId> | ||
| <artifactId>jakarta.json-api</artifactId> | ||
| <version>2.0.1</version> | ||
| </dependency> |
There was a problem hiding this comment.
jakarta.json-api is API-only; add a JSON-P implementation to avoid runtime failures.
Without a provider, Json.create(...) will fail at runtime.
Add:
<dependency>
<groupId>jakarta.json</groupId>
<artifactId>jakarta.json-api</artifactId>
<version>2.0.1</version>
</dependency>
+ <dependency>
+ <groupId>org.glassfish</groupId>
+ <artifactId>jakarta.json</artifactId>
+ <version>2.0.1</version>
+ </dependency>🤖 Prompt for AI Agents
In financial-module-system/egf-instrument/pom.xml around lines 63-66, you
currently declare only the jakarta.json-api (API-only) which will cause
Json.create(...) to fail at runtime without a provider; add a JSON‑P
implementation dependency next to the API entry (for example a GlassFish
provider such as org.glassfish:jakarta.json or org.glassfish:jakarta.json-ri
with an appropriate version) so a runtime provider is present, and ensure the
implementation version is compatible with the API.
| private static final Logger logger = LoggerFactory.getLogger(InstrumentAccountCodeESRepository.class); | ||
| private static final String INDEX_NAME = "instrumentaccountcode"; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid hardcoding index names; externalize via config/alias for ES v8.
Use a property (with a sane default) to allow alias-based rollovers and environment overrides.
- private static final String INDEX_NAME = "instrumentaccountcode";
+ @Value("${es.index.instrumentaccountcode:instrumentaccountcode}")
+ private String indexName;And update usage:
- return esHttpClient.search(INDEX_NAME, query, InstrumentAccountCode.class);
+ return esHttpClient.search(indexName, query, InstrumentAccountCode.class);📝 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.
| private static final Logger logger = LoggerFactory.getLogger(InstrumentAccountCodeESRepository.class); | |
| private static final String INDEX_NAME = "instrumentaccountcode"; | |
| private static final Logger logger = LoggerFactory.getLogger(InstrumentAccountCodeESRepository.class); | |
| @Value("${es.index.instrumentaccountcode:instrumentaccountcode}") | |
| private String indexName; | |
| // … | |
| public List<InstrumentAccountCode> searchInstrumentAccountCodes(Query query) { | |
| // … | |
| return esHttpClient.search(indexName, query, InstrumentAccountCode.class); | |
| } |
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java
around lines 18-20, the ES index name is hardcoded; replace it with a
configurable property (with a sensible default) and use an alias for rollovers.
Add a configuration property (e.g., egf.instrument.account.index or similar)
wired into the repository via constructor injection or @Value, defaulting to
"instrumentaccountcode", and update all usages in this class to reference that
injected property; ensure any index creation/lookup logic treats the value as an
alias so environment overrides and ES v8 rollover strategies work without code
changes.
| @Autowired | ||
| private ESHttpClient esHttpClient; | ||
|
|
||
| ObjectMapper mapper = new ObjectMapper(); | ||
| // JSON from file to Object | ||
| try { | ||
| instrumentAccountCode = mapper.readValue(hit.getSourceAsString(), InstrumentAccountCode.class); | ||
| } catch (JsonParseException e1) { | ||
| // TODO Auto-generated catch block | ||
| logger.error("Error occurred while parsing JSON: " + e1.getMessage()); | ||
| } catch (JsonMappingException e1) { | ||
| // TODO Auto-generated catch block | ||
| logger.error("JSON mapping exception occurred: " + e1.getMessage()); | ||
| } catch (IOException e1) { | ||
| // TODO Auto-generated catch block | ||
| logger.error("IO exception occured: " + e1.getMessage()); | ||
| } | ||
| @Autowired | ||
| private ESQueryFactory esQueryFactory; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer constructor injection over field injection.
Improves immutability, testability, and avoids reflection-based wiring.
- @Autowired
- private ESHttpClient esHttpClient;
-
- @Autowired
- private ESQueryFactory esQueryFactory;
+ private final ESHttpClient esHttpClient;
+ private final ESQueryFactory esQueryFactory;
+
+ @Autowired
+ public InstrumentAccountCodeESRepository(ESHttpClient esHttpClient, ESQueryFactory esQueryFactory) {
+ this.esHttpClient = esHttpClient;
+ this.esQueryFactory = esQueryFactory;
+ }📝 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.
| @Autowired | |
| private ESHttpClient esHttpClient; | |
| ObjectMapper mapper = new ObjectMapper(); | |
| // JSON from file to Object | |
| try { | |
| instrumentAccountCode = mapper.readValue(hit.getSourceAsString(), InstrumentAccountCode.class); | |
| } catch (JsonParseException e1) { | |
| // TODO Auto-generated catch block | |
| logger.error("Error occurred while parsing JSON: " + e1.getMessage()); | |
| } catch (JsonMappingException e1) { | |
| // TODO Auto-generated catch block | |
| logger.error("JSON mapping exception occurred: " + e1.getMessage()); | |
| } catch (IOException e1) { | |
| // TODO Auto-generated catch block | |
| logger.error("IO exception occured: " + e1.getMessage()); | |
| } | |
| @Autowired | |
| private ESQueryFactory esQueryFactory; | |
| private final ESHttpClient esHttpClient; | |
| private final ESQueryFactory esQueryFactory; | |
| @Autowired | |
| public InstrumentAccountCodeESRepository(ESHttpClient esHttpClient, ESQueryFactory esQueryFactory) { | |
| this.esHttpClient = esHttpClient; | |
| this.esQueryFactory = esQueryFactory; | |
| } |
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java
around lines 21 to 26, the ESHttpClient and ESQueryFactory are injected via
field @Autowired; replace field injection with constructor injection: make the
two fields private final, remove the field @Autowired annotations, add a single
constructor that accepts ESHttpClient and ESQueryFactory and assigns them to the
final fields (you can annotate the constructor with @Autowired or omit the
annotation if using Spring's single-constructor autowiring), and update any unit
tests to inject mocks via the constructor or adjust test setup accordingly.
| public Pagination<InstrumentAccountCode> search(InstrumentAccountCodeSearchContract instrumentAccountCodeSearchContract) { | ||
| try { | ||
| validateSortBy(instrumentAccountCodeSearchContract.getSortBy()); | ||
|
|
||
| Map<String, Object> query = esQueryFactory.buildInstrumentAccountCodeQuery(instrumentAccountCodeSearchContract); | ||
|
|
||
| logger.debug("Searching instrument account codes with query: {}", query); | ||
|
|
||
| return esHttpClient.search(INDEX_NAME, query, InstrumentAccountCode.class); | ||
|
|
||
| } catch (Exception e) { | ||
| logger.error("Error occurred while searching instrument account codes: {}", e.getMessage(), e); | ||
| return new Pagination<>(); | ||
| } | ||
|
|
||
| page.setTotalResults(Long.valueOf(searchResponse.getHits().getTotalHits()).intValue()); | ||
| page.setPagedData(instrumentAccountCodes); | ||
|
|
||
| return page; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't swallow search failures by returning an empty page.
Catching Exception and returning a blank Pagination hides outages and misleads callers.
- try {
+ try {
validateSortBy(instrumentAccountCodeSearchContract.getSortBy());
Map<String, Object> query = esQueryFactory.buildInstrumentAccountCodeQuery(instrumentAccountCodeSearchContract);
logger.debug("Searching instrument account codes with query: {}", query);
- return esHttpClient.search(INDEX_NAME, query, InstrumentAccountCode.class);
+ return esHttpClient.search(indexName, query, InstrumentAccountCode.class);
- } catch (Exception e) {
- logger.error("Error occurred while searching instrument account codes: {}", e.getMessage(), e);
- return new Pagination<>();
+ } catch (RuntimeException e) {
+ logger.error("ES search failed for index={}, sortBy={}, error={}", indexName,
+ instrumentAccountCodeSearchContract.getSortBy(), e.getMessage(), e);
+ throw e; // or wrap in a domain-specific exception if preferred
}If API compatibility mandates a non-throwing path, at least return an empty Pagination with non-null item list and propagated page/offset.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/InstrumentAccountCodeESRepository.java
around lines 27-41, the catch block swallows all Exceptions and returns a blank
Pagination which hides failures; either let the exception propagate (remove the
broad catch) or, if API requires not throwing, construct and return a Pagination
that preserves request pagination metadata and a non-null empty items list while
still logging the full error. Specifically, replace the generic catch with
either: 1) rethrow the exception after logging, or 2) build a new Pagination
with items = Collections.emptyList(), totalResults = 0, page and pageSize (or
offset/limit) copied from instrumentAccountCodeSearchContract, set any other
pagination fields consistently, log the full exception (logger.error with the
exception object) and return that Pagination.
| private static final String INDEX_NAME = "surrenderreason"; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using an alias or configurable index name
Hardcoding "surrenderreason" reduces flexibility for ILM/rollovers and env-specific prefixes. Prefer an alias or property-driven name.
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java
around lines 19-20, replace the hardcoded INDEX_NAME = "surrenderreason" with a
configurable or alias-driven value: inject the index name from application
properties (or environment) with a sensible default and/or resolve an
Elasticsearch alias at startup; update the constant to be initialized from that
injected property (or a config class) so ILM rollovers and env-specific prefixes
can be used, and ensure related initialization/tests reference the configurable
name rather than the hardcoded string.
| public Pagination<SurrenderReason> search(SurrenderReasonSearchContract surrenderReasonSearchContract) { | ||
| try { | ||
| validateSortBy(surrenderReasonSearchContract.getSortBy()); | ||
|
|
||
| Map<String, Object> query = esQueryFactory.buildSurrenderReasonQuery(surrenderReasonSearchContract); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: null-check request to avoid NPEs
Defensively guard against a null SurrenderReasonSearchContract.
- public Pagination<SurrenderReason> search(SurrenderReasonSearchContract surrenderReasonSearchContract) {
+ public Pagination<SurrenderReason> search(SurrenderReasonSearchContract surrenderReasonSearchContract) {
+ if (surrenderReasonSearchContract == null) return new Pagination<>();📝 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 Pagination<SurrenderReason> search(SurrenderReasonSearchContract surrenderReasonSearchContract) { | |
| try { | |
| validateSortBy(surrenderReasonSearchContract.getSortBy()); | |
| Map<String, Object> query = esQueryFactory.buildSurrenderReasonQuery(surrenderReasonSearchContract); | |
| public Pagination<SurrenderReason> search(SurrenderReasonSearchContract surrenderReasonSearchContract) { | |
| if (surrenderReasonSearchContract == null) return new Pagination<>(); | |
| try { | |
| validateSortBy(surrenderReasonSearchContract.getSortBy()); | |
| Map<String, Object> query = esQueryFactory.buildSurrenderReasonQuery(surrenderReasonSearchContract); | |
| // … |
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java
around lines 27 to 31, the method search(...) does not guard against a null
SurrenderReasonSearchContract; add a null-check at the start of the method
(e.g., if the contract is null) and fail fast by throwing an
IllegalArgumentException with a clear message like
"surrenderReasonSearchContract must not be null" (or alternatively return an
empty Pagination) before calling validateSortBy or using the contract in any
operations so you avoid NPEs.
| } catch (Exception e) { | ||
| logger.error("Error occurred while searching surrender reasons: {}", e.getMessage(), e); | ||
| return new Pagination<>(); | ||
| } | ||
|
|
||
| page.setTotalResults(Long.valueOf(searchResponse.getHits().getTotalHits()).intValue()); | ||
| page.setPagedData(surrenderReasons); | ||
|
|
||
| return page; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Don’t swallow ES errors; log context and confirm fallback semantics
Returning an empty Pagination on any Exception masks outages and yields misleading 200/empty responses. If empty fallback is intentional, log richer context.
Minimal logging improvement:
- } catch (Exception e) {
- logger.error("Error occurred while searching surrender reasons: {}", e.getMessage(), e);
+ } catch (Exception e) {
+ logger.error("ES search failed for index {} with params {}: {}",
+ INDEX_NAME, surrenderReasonSearchContract, e.getMessage(), e);
return new Pagination<>();
}If policy allows, prefer surfacing a domain error up the stack instead of silent empty results.
📝 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.
| } catch (Exception e) { | |
| logger.error("Error occurred while searching surrender reasons: {}", e.getMessage(), e); | |
| return new Pagination<>(); | |
| } | |
| page.setTotalResults(Long.valueOf(searchResponse.getHits().getTotalHits()).intValue()); | |
| page.setPagedData(surrenderReasons); | |
| return page; | |
| } | |
| } catch (Exception e) { | |
| logger.error("ES search failed for index {} with params {}: {}", | |
| INDEX_NAME, surrenderReasonSearchContract, e.getMessage(), e); | |
| return new Pagination<>(); | |
| } |
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java
around lines 37-41, the catch block swallows all Exceptions and returns an empty
Pagination which masks ES outages; instead, log richer context (search
parameters, pagination offset/limit, tenantId or request identifiers if
available) along with the full exception, and change the fallback behavior to
surface the error: either rethrow a domain/data-access runtime exception (e.g.,
a custom SurrenderReasonQueryException) wrapping the original exception or, if
an empty result is truly intended, add an explicit comment and a WARN-level log
that includes the full context and stacktrace; implement one of these two
behaviors (prefer throwing a wrapped exception).
| private void validateSortBy(String sortBy) { | ||
| if (sortBy != null && !sortBy.trim().isEmpty()) { | ||
| validateSortByOrder(sortBy); | ||
| validateEntityFieldName(sortBy, SurrenderReasonEntity.class); | ||
| } | ||
|
|
||
| final BoolQueryBuilder boolQueryBuilder = elasticSearchQueryFactory.searchSurrenderReason(criteria); | ||
| SearchRequestBuilder searchRequestBuilder = esClient.prepareSearch(SurrenderReason.class.getSimpleName().toLowerCase()) | ||
| .setTypes(SurrenderReason.class.getSimpleName().toLowerCase()); | ||
| if (!orderByList.isEmpty()) | ||
| for (String orderBy : orderByList) | ||
| searchRequestBuilder = searchRequestBuilder.addSort(orderBy.split(" ")[0], | ||
| orderBy.split(" ")[1].equalsIgnoreCase("asc") ? SortOrder.ASC : SortOrder.DESC); | ||
|
|
||
| searchRequestBuilder.setQuery(boolQueryBuilder); | ||
| return searchRequestBuilder; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sort validation may pass fields that aren’t sortable in ES 8
validateEntityFieldName checks entity fields, not ES mappings. Sorting on analyzed text fields will fail at runtime; in ES 8 you typically need keyword/numeric/date. Validate against a whitelist of sortable fields or mapping metadata.
Option: restrict sortBy to a constant set of known sortable fields:
- private void validateSortBy(String sortBy) {
+ private void validateSortBy(String sortBy) {
if (sortBy != null && !sortBy.trim().isEmpty()) {
validateSortByOrder(sortBy);
- validateEntityFieldName(sortBy, SurrenderReasonEntity.class);
+ validateEntityFieldName(sortBy, SurrenderReasonEntity.class);
+ // additionally ensure only sortable fields are allowed (keyword/numeric/date)
+ validateSortableFields(sortBy, Set.of(
+ "id", "name.keyword", "active", "createdDate", "lastModifiedDate"
+ ));
}
}To confirm current behavior of base helpers:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'class\s+ESRepository\b' -A200 -g '!**/target/**'
rg -nP 'validate(EntityFieldName|SortByOrder)\s*\(' -n -A3 -B3
rg -nP 'buildSurrenderReasonQuery\s*\(' -n -A2 -B2Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Listing first 50 Java files in repository ---"
find . -type f -name '*.java' | head -n 50
echo "--- Searching for ESRepository class definition ---"
rg -nP 'class\s+ESRepository' -C3
echo "--- Searching for validateEntityFieldName usage ---"
rg -nP 'validateEntityFieldName' -C3
echo "--- Searching for validateSortByOrder usage ---"
rg -nP 'validateSortByOrder' -C3
echo "--- Searching for buildSurrenderReasonQuery definition ---"
rg -nP 'buildSurrenderReasonQuery' -C3Length of output: 4976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Locating SurrenderReasonESRepository.java ---"
find . -type f -name 'SurrenderReasonESRepository.java'
echo "--- Showing validateSortBy in that file ---"
file_path=$(find . -type f -name 'SurrenderReasonESRepository.java')
sed -n '1,200p' "$file_path" | sed -n '1,50p'
echo "--- Searching repository for ESRepository base class ---"
rg -nP 'class\s+ESRepository' -n --glob '*.java'
echo "--- Searching for validateEntityFieldName definition and usages ---"
rg -nP 'validateEntityFieldName' -n --glob '*.java'
echo "--- Searching for validateSortByOrder definition and usages ---"
rg -nP 'validateSortByOrder' -n --glob '*.java'
echo "--- Searching for buildSurrenderReasonQuery ---"
rg -nP 'buildSurrenderReasonQuery' -n --glob '*.java'Length of output: 25254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display ESRepository implementation to inspect validate methods
sed -n '1,200p' financial-module-system/egf-master/src/main/java/org/egov/common/persistence/repository/ESRepository.javaLength of output: 4186
Enforce ES-specific sortable field validation in validateSortBy
validateEntityFieldName only ensures the Java entity has the field, not that Elasticsearch mapping allows sorting on it. Sorting on analyzed text fields (e.g. name) will fail at runtime under ES 8. Introduce a whitelist of sortable fields (keyword/numeric/date) or retrieve mapping metadata before accepting sortBy.
Option: restrict sortBy to known sortable fields:
private void validateSortBy(String sortBy) {
if (sortBy != null && !sortBy.trim().isEmpty()) {
validateSortByOrder(sortBy);
validateEntityFieldName(sortBy, SurrenderReasonEntity.class);
+ validateSortableFields(sortBy, Set.of(
+ "id", "name.keyword", "active", "createdDate", "lastModifiedDate"
+ ));
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/domain/repository/SurrenderReasonESRepository.java
around lines 43-48, the current validateSortBy only checks the Java entity field
but not whether the field is sortable in Elasticsearch; add an ES-specific
validation by either (A) maintaining a private static whitelist/set of known
sortable fields (keyword, numeric, date) and validate sortBy against that set
after trimming, or (B) querying the index mapping at startup to build a set of
sortable fields from the mapping metadata and validate against it; update
validateSortBy to call this ES-sortable check (and keep the existing
entity-field check), throw a clear IllegalArgumentException when not allowed,
and add/update unit tests to cover allowed and disallowed sortBy values.
| private ResponseInfo getResponseInfo(RequestInfo requestInfo) { | ||
| return ResponseInfo.builder().apiId(requestInfo.getApiId()).ver(requestInfo.getVer()) | ||
| .ts(new SimpleDateFormat("dd-MM-yyyy HH:mm:ss").format(new Date())).resMsgId(requestInfo.getMsgId()) | ||
| .resMsgId(PLACEHOLDER).status(PLACEHOLDER).build(); | ||
| .ts(System.currentTimeMillis()).resMsgId(PLACEHOLDER) | ||
| .status(PLACEHOLDER).build(); | ||
| } |
There was a problem hiding this comment.
Stop hardcoding ResponseInfo fields; use request msgId and a real status.
Hardcoded resMsgId/status breaks traceability and may confuse clients expecting "successful"/"failed". Use requestInfo.getMsgId() with UUID fallback and set status to "successful" for 2xx paths.
Apply:
- return ResponseInfo.builder().apiId(requestInfo.getApiId()).ver(requestInfo.getVer())
- .ts(System.currentTimeMillis()).resMsgId(PLACEHOLDER)
- .status(PLACEHOLDER).build();
+ return ResponseInfo.builder()
+ .apiId(requestInfo.getApiId())
+ .ver(requestInfo.getVer())
+ .ts(System.currentTimeMillis())
+ .resMsgId(requestInfo != null && requestInfo.getMsgId() != null
+ ? requestInfo.getMsgId()
+ : java.util.UUID.randomUUID().toString())
+ .status("successful")
+ .build();📝 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.
| private ResponseInfo getResponseInfo(RequestInfo requestInfo) { | |
| return ResponseInfo.builder().apiId(requestInfo.getApiId()).ver(requestInfo.getVer()) | |
| .ts(new SimpleDateFormat("dd-MM-yyyy HH:mm:ss").format(new Date())).resMsgId(requestInfo.getMsgId()) | |
| .resMsgId(PLACEHOLDER).status(PLACEHOLDER).build(); | |
| .ts(System.currentTimeMillis()).resMsgId(PLACEHOLDER) | |
| .status(PLACEHOLDER).build(); | |
| } | |
| private ResponseInfo getResponseInfo(RequestInfo requestInfo) { | |
| return ResponseInfo.builder() | |
| .apiId(requestInfo.getApiId()) | |
| .ver(requestInfo.getVer()) | |
| .ts(System.currentTimeMillis()) | |
| .resMsgId(requestInfo != null && requestInfo.getMsgId() != null | |
| ? requestInfo.getMsgId() | |
| : java.util.UUID.randomUUID().toString()) | |
| .status("successful") | |
| .build(); | |
| } |
🤖 Prompt for AI Agents
In
financial-module-system/egf-instrument/src/main/java/org/egov/egf/instrument/web/controller/InstrumentAccountCodeController.java
around lines 176 to 180, the getResponseInfo method currently hardcodes resMsgId
and status; change it to use requestInfo.getMsgId() (and fall back to
UUID.randomUUID().toString() when null/blank) for resMsgId, and set status to
"successful" for normal 2xx responses; preserve apiId, ver and ts as before and
ensure you handle a null requestInfo safely before accessing getMsgId().
1 participant
Summary by CodeRabbit