[NAE-2168] Optimize bulk index for elasticsearch in v7 #339
Conversation
- Introduced `LocalDateTimeJsonSerializer` and `LocalDateTimeJsonDeserializer` for improved LocalDateTime handling. - Enhanced bulk indexing in ElasticIndexService: - Added new methods for reindexing cases and tasks into Elasticsearch. - Introduced batch processing for efficient indexing with log monitoring. - Removed deprecated `bulkIndex(List, Class, String...)` method. - Updated ElasticTask and IndexCaseEvent to replace `stringId` with `id` field, simplifying object mapping. - Improved task and case processing for better compatibility with MongoDB and Elasticsearch. - Updated task entity construction in response body for consistency.
Replaced try-with-resources for CloseableIterator with direct Iterator usage for reindexing cases and tasks. Enhanced error reporting in bulk indexing by including detailed failure messages. Minor adjustments to method calls for improved clarity and consistency.
Updated repository methods to use `id` instead of `stringId` for consistency and clarity. Adjusted associated service and task management logic to ensure compatibility with the updated repository interface.
Moved the ObjectMapper configuration from ElasticsearchConfiguration to ElasticIndexService. This improves encapsulation by localizing the mapping logic within the service, reducing unnecessary bean exposure.
Introduced a custom JsonpMapper to handle Java 8 `LocalDateTime` serialization and deserialization using Jackson. This ensures proper JSON handling of datetime objects in Elasticsearch operations.
WalkthroughMigrate identifier usage from stringId/mongoId to id across domains, repos, services, events, mappings and tests; add multi-host SSL/token/basic-auth Elasticsearch client and Jackson-backed JsonpMapper; add millisecond-safe LocalDateTime (de)serializers; implement Mongo-driven, batched bulk reindex API and controller endpoint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin Client
participant API as ElasticController
participant IndexSvc as IElasticIndexService
participant Mongo as MongoDB
participant Map as Mapping Services
participant ES as Elasticsearch
Admin->>API: POST /elastic/bulkReindex {IndexParams}
API->>IndexSvc: bulkIndex(indexAll,lastRun,caseBatchSize,taskBatchSize)
rect rgb(245,248,255)
note over IndexSvc: Case reindex (paged via Mongo)
IndexSvc->>Mongo: Query Cases (page by lastModified)
Mongo-->>IndexSvc: Case batch
IndexSvc->>Map: map Cases -> ElasticCase
Map-->>IndexSvc: ElasticCase list
IndexSvc->>ES: Bulk index cases (BulkRequest)
ES-->>IndexSvc: BulkResponse
IndexSvc->>IndexSvc: on partial failures -> split & retry
end
rect rgb(245,255,245)
note over IndexSvc: Task reindex (batched by caseIds)
IndexSvc->>Mongo: Query Tasks (by caseIds)
Mongo-->>IndexSvc: Task batch
IndexSvc->>Map: map Tasks -> ElasticTask
Map-->>IndexSvc: ElasticTask list
IndexSvc->>ES: Bulk index tasks (BulkRequest)
ES-->>IndexSvc: BulkResponse (check/split on failures)
end
IndexSvc-->>API: Completed
API-->>Admin: 200 OK / message
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (1)
38-45: Validate and key by entity ID; current null-check can miss null id and cause runtime errorsYou’re guarding on task.getTask().getTaskId(), but later use task.getId() for repository operations. If id is null but taskId is not, repository.findById(null) will throw. Prefer validating (and queuing by) the entity id to align with the rest of the changes.
Apply:
- if (task.getTask().getTaskId() == null) { + if (task.getTask().getId() == null) { throw new IllegalArgumentException("Task id cannot be null"); } - - String taskId = task.getTaskId(); + String taskId = task.getTask().getId(); log.debug("Scheduling operation for task: {}", taskId);If the queue is intentionally keyed by a different business key, keep the queue key as-is, but still validate task.getTask().getId() for null to prevent IllegalArgumentException from downstream repository calls.
🧹 Nitpick comments (27)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java (2)
20-20: If you keep the field, make it null-safe and immutableThe current initialization uses
Arrays.asList, which returns a fixed-size, array-backed list and does not handle a null source. Prefer an unmodifiable list and guard against nulls.Apply this diff:
- this.caseValue = Arrays.asList(fullTextValue); + this.caseValue = (fullTextValue == null) ? null : List.of(fullTextValue);Notes:
- If any element in
fullTextValuecan be null,List.ofwill throw NPE. In that case, use:
this.caseValue = (fullTextValue == null) ? null : Arrays.stream(fullTextValue).filter(Objects::nonNull).toList();
(and addimport java.util.Objects;)- Consider suppressing Lombok’s public setter for
caseValueto avoid external mutation:
@Setter(AccessLevel.NONE)on the field (and importlombok.Setterandlombok.AccessLevel).
8-8: Import cleanup opportunityIf you adopt
List.of(...)as suggested on Line 20 (or remove the field entirely),Arraysbecomes unused. Please remove this import to keep the file tidy.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.java (1)
28-32: Expose mapping for caseValue — LGTM; consider adding a keyword subfield for exact matches/aggregationsThe override and
@Field(type = Text)mapping for aList<String>is correct for analyzed full-text queries.If you also need exact matching, sorting, or aggregations on
caseValue, consider a multifield mapping with a keyword subfield (keeps analyzed text while enabling exact queries):Example (outside the selected range):
import org.springframework.data.elasticsearch.annotations.MultiField; import org.springframework.data.elasticsearch.annotations.InnerField; import static org.springframework.data.elasticsearch.annotations.FieldType.Keyword; @Override @MultiField( mainField = @Field(type = Text), otherFields = { @InnerField(suffix = "keyword", type = Keyword) } ) public List<String> getCaseValue() { return super.getCaseValue(); }This will produce
caseValue(text) andcaseValue.keywordfor exact match and aggregations without incurringfielddatacosts ontext.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonSerializer.java (1)
1-18: Preserve fractional precision and align with standard ISO formatUsing a fixed
SSSpattern truncates to milliseconds. To stay symmetric with typical parsing (optional fractional seconds) and avoid precision loss, prefer ISO formatter (variable fraction digits). Minor: add a null guard (Jackson usually skips nulls, but harmless).Apply:
- private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS"); + // Use ISO_LOCAL_DATE_TIME to preserve fractional precision when present and keep symmetry with parsing + private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ISO_LOCAL_DATE_TIME; @@ - public void serialize(LocalDateTime value, JsonGenerator gen, SerializerProvider serializers) throws IOException { - gen.writeString(FORMATTER.format(value)); + public void serialize(LocalDateTime value, JsonGenerator gen, SerializerProvider serializers) throws IOException { + if (value == null) { + gen.writeNull(); + return; + } + gen.writeString(FORMATTER.format(value));application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/IndexParams.java (1)
13-26: Add validation to prevent zero/negative batch sizes and document null-as-default semanticsCurrently, clients can submit 0 or negative values, which can break batching logic or cause degenerate loops. If null is intended to mean "use server-configured defaults" (from DataConfigurationProperties), keep Integer but validate when provided.
Proposed change:
package com.netgrif.application.engine.elastic.web.requestbodies; import lombok.Data; +import jakarta.validation.constraints.Min; /** * Represents the parameters to configure the indexing operation. * This class allows customization of batch sizes for cases and tasks, * as well as the option to index all data. */ @Data public class IndexParams { /** * Determines whether to index all available data. Default is {@code false}. */ private boolean indexAll = false; /** - * Specifies the batch size for cases during indexing. Default is {@code 5000}. + * Specifies the batch size for cases during indexing. Default is {@code 5000}. + * If null, the server-side default is used. When provided, must be >= 1. */ - private Integer caseBatchSize = 5000; + @Min(1) + private Integer caseBatchSize = 5000; /** - * Specifies the batch size for tasks during indexing. Default is {@code 20000}. + * Specifies the batch size for tasks during indexing. Default is {@code 20000}. + * If null, the server-side default is used. When provided, must be >= 1. */ - private Integer taskBatchSize = 20000; + @Min(1) + private Integer taskBatchSize = 20000; }application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/ElasticCaseRepository.java (1)
10-12: Clarify and disambiguate your custom deleteAllById
- Rename the parameter in
long countByIdAndLastModified(String stringId, long lastUpdated);
to- long countByIdAndLastModified(String stringId, long lastUpdated); + long countByIdAndLastModified(String id, long lastUpdated);- Your custom
void deleteAllById(String id)currently overloads
CrudRepository.deleteAllById(Iterable<ID>)and may confuse readers. You have two options:
• Rename it tovoid deleteAllByIdEquals(String id)so it clearly targets the entity’s id field.
• Or add a Javadoc explaining “Deletes all documents whoseidproperty equals the given value.”Call-site locations you’ll need to update if you rename the method:
- ElasticTaskQueueManager.java (repository.deleteAllById(task.getId()))
- ElasticCaseService.java (repository.deleteAllById(caseId) and repository.deleteAllById(useCase.getId()))
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (2)
89-91: Ensure creationDateSortable matches the truncated creationDateYou truncate creationDate to milliseconds but compute creationDateSortable from the original (potentially higher-precision) timestamp. Use the truncated value for consistency across fields.
- creationDate = useCase.getCreationDate().truncatedTo(ChronoUnit.MILLIS); - creationDateSortable = Timestamp.valueOf(useCase.getCreationDate()).getTime(); + creationDate = useCase.getCreationDate().truncatedTo(ChronoUnit.MILLIS); + creationDateSortable = Timestamp.valueOf(creationDate).getTime();
109-111: Null-safe version incrementversion is a Long. Using version++ will NPE if it is null. Guard the first increment.
- version++; + version = (version == null ? 0L : version) + 1L;If version is guaranteed non-null by upstream code, please confirm; otherwise this guard avoids sporadic NPEs during updates.
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (3)
160-166: Doc nit: url is now multi-host; update commentThe field is now a List. Adjust the comment to avoid confusion.
- /** - * Hostname for the Elasticsearch server. - */ + /** + * Hostnames or base URLs for the Elasticsearch cluster (multi-host support). + */
302-339: maxPoolSize default does not track customized size at runtimemaxPoolSize is initialized as size * 10 at field init time; if size is overridden via configuration, maxPoolSize won’t recompute. Either document that it’s an independent property or compute it lazily/at init if not explicitly set.
Option A (document-only): Update the javadoc to clarify it’s an independent configurable property.
Option B (auto-compute when unset):
@Data public static class ExecutorProperties { private int size = 50; private int timeout = 5; - private int maxPoolSize = size * 10; + private Integer maxPoolSize; // null means compute from size private boolean allowCoreThreadTimeOut = true; private int keepAliveSeconds = 30; private String threadNamePrefix = "ElasticTaskExecutor-"; + + @PostConstruct + void init() { + if (maxPoolSize == null) { + maxPoolSize = size * 10; + } + } }
162-166: Clarify auth precedence and invalid combinations (token vs username/password)Given both token and username/password are supported, define precedence (e.g., token first) and validate that mutually exclusive combinations are not used simultaneously.
Option: add a simple validation hook in ElasticsearchProperties:
@PostConstruct public void init() { + if (token != null && (username != null || password != null)) { + throw new IllegalArgumentException("Configure either token or username/password, not both."); + } indexSettings.putIfAbsent("max_result_window", 10000000); mappingSettings.putIfAbsent("date_detection", false); ... }If your ElasticsearchConfiguration already handles this precedence, please confirm and ignore this suggestion.
Also applies to: 177-191
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ReindexTest.groovy (1)
87-87: Switch toidlooks correct; consider idiomatic Groovy string interpolationGood move aligning the query with the new
idfield. Minor readability nit: Groovy interpolation avoids manual concatenation and escaping.Apply this diff for readability:
- request.query = "id:\"" + it.getStringId() + "\"" + request.query = "id:\"${it.getStringId()}\""application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.java (1)
153-153: Use fully-qualified method reference to avoid class ambiguityThere are two ElasticTask types in play (adapter vs. objects). Using the simple name in the method reference risks confusion and accidental binding. Fully-qualify the method reference to the adapter class to make intent explicit and reduce the chance of future breakage.
Apply this diff to make the reference unambiguous:
- taskPage = taskService.findAllById(indexedTasks.get().map(ElasticTask::getId).collect(Collectors.toList())); + taskPage = taskService.findAllById( + indexedTasks.get() + .map(com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask::getId) + .collect(Collectors.toList()) + );If you want to go one step further and eliminate the raw cast above, I can propose a typed variant for the unwrapping as a follow-up.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java (1)
81-81: ID migration aligns with the ES shiftInitializing
idfromtask.getStringId()is consistent with the repo-wide move toid. Note there is also ataskIdfield set to the same value (Line 83); if duplication isn’t required by downstream consumers/mappings, consider consolidating later to avoid divergence.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
260-265: MIDNIGHT normalization is consistent with test updatesSwitching Date-only indexing to midnight is coherent with the tests and prevents the Noon skew. No functional issues spotted.
If this normalization is used elsewhere, consider extracting a small helper to avoid drift:
- Example:
private static LocalDateTime atStartOfDay(LocalDate date) { return LocalDateTime.of(date, LocalTime.MIDNIGHT); }application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (2)
63-66: Avoid potential NPE on queue.poll()Objects.requireNonNull(queue.poll()) can theoretically NPE under contention. Mirror the safer pattern used in scheduleNextTask().
- elasticTaskExecutor.submit(Objects.requireNonNull(queue.poll())); + Runnable first = queue.poll(); + if (first != null) { + elasticTaskExecutor.submit(first); + } else { + activeTasks.remove(taskId); + }
129-135: Duplicate handling: align logs with ID and keep stacktrace in error logging
- Log after reindex should include the id for consistency.
- Preserve stacktrace in the error log.
- repository.save((com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask) task); - log.debug("[{}]: Task \"{}\" indexed", task.getCaseId(), task.getTitle()); + repository.save((com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask) task); + log.debug("[{}]: Task \"{}\" [{}] indexed", task.getCaseId(), task.getTitle(), task.getId()); } catch (RuntimeException e) { - log.error("Elastic executor was killed before finish: {}", e.getMessage()); + log.error("Elastic executor was killed before finish", e); }application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticIndexService.java (2)
8-8: Time type choice: prefer Instant over LocalDateTimeUnless you explicitly need locale-specific wall time, Instant avoids timezone and DST ambiguity in distributed systems.
17-17: Document bulkIndex parameter nullability and consider a parameter objectTo make the API contract explicit (call sites in ReindexingTask.java and ElasticController.java already pass
nullfor these args), annotate thebulkIndexsignature:• In application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticIndexService.java:
+ import org.springframework.lang.Nullable; @@ - void bulkIndex(boolean indexAll, LocalDateTime lastRun, Integer caseBatchSize, Integer taskBatchSize); + void bulkIndex(boolean indexAll, + @Nullable LocalDateTime lastRun, + @Nullable Integer caseBatchSize, + @Nullable Integer taskBatchSize);(Optional) Propagate the same
@Nullableannotations to the@Overridein ElasticIndexService.java to keep documentation consistent.(Optional refactor) Introduce a params object to group these arguments and improve readability:
public record BulkIndexParams( boolean indexAll, @Nullable LocalDateTime lastRun, @Nullable Integer caseBatchSize, @Nullable Integer taskBatchSize ) {} void bulkIndex(BulkIndexParams params);application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/ElasticTaskRepository.java (1)
14-14: Parameter name is misleading; it’s an id, not a taskIdMinor clarity fix: rename the parameter to id to match the property in the derived query and the rest of the codebase.
- void deleteAllById(String taskId); + void deleteAllById(String id);application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java (1)
100-105: Fix off-by-one page calculationWhen count is a multiple of page size, this adds an extra empty page. Use ceiling division.
- long numOfPages = (count / elasticsearchProperties.getReindexExecutor().getSize()) + 1; + long size = elasticsearchProperties.getReindexExecutor().getSize(); + long numOfPages = (count + size - 1) / size;application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (1)
104-121: Upsert-by-id path looks good; optionally publish event in duplicate path as wellThe Optional-based upsert is correct. Consider publishing the IndexCaseEvent in the duplicate reindex path to keep downstream listeners consistent.
} catch (InvalidDataAccessApiUsageException ignored) { log.debug("[" + useCase.getId() + "]: Case \"" + useCase.getTitle() + "\" has duplicates, will be reindexed"); repository.deleteAllById(useCase.getId()); repository.save((com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticCase) useCase); - log.debug("[" + useCase.getId() + "]: Case \"" + useCase.getTitle() + "\" indexed"); + log.debug("[" + useCase.getId() + "]: Case \"" + useCase.getTitle() + "\" indexed"); + publisher.publishEvent(new IndexCaseEvent(useCase)); }nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonDeserializer.java (2)
15-20: Broaden fraction parsing to nanos for better interoperabilityParsing only MILLI_OF_SECOND (1–3 digits) is strict; upstream sources may send micro/nanosecond precision. Switching to NANO_OF_SECOND (0–9) keeps compatibility with the existing serializer while accepting a wider range.
Apply this diff:
- private static final DateTimeFormatter FORMATTER = new DateTimeFormatterBuilder() - .appendPattern("yyyy-MM-dd'T'HH:mm:ss") - .optionalStart() - .appendFraction(ChronoField.MILLI_OF_SECOND, 1, 3, true) - .optionalEnd() - .toFormatter(); + private static final DateTimeFormatter FORMATTER = new DateTimeFormatterBuilder() + .appendPattern("yyyy-MM-dd'T'HH:mm:ss") + .optionalStart() + .appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true) + .optionalEnd() + .toFormatter();
23-29: Trim input to avoid spurious parse failures on whitespaceDefensive trim of the incoming string prevents failures on values like "2025-08-19T10:01:02.123 " and keeps null/empty handling intact.
Apply this diff:
- String value = p.getValueAsString(); - if (value == null || value.isEmpty()) { + String value = p.getValueAsString(); + if (value == null) { return null; } - return LocalDateTime.parse(value, FORMATTER); + value = value.trim(); + if (value.isEmpty()) { + return null; + } + return LocalDateTime.parse(value, FORMATTER);application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticsearchConfiguration.java (1)
61-83: Validate multi-host inputs earlyConsider failing fast if
elasticsearchProperties.getUrl()is null/empty to avoid a confusing client configuration error downstream. Optional, since it's environment/config guarded.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (2)
617-623: ObjectMapper configuration is fine; consider reusing itThe configured
objectMapperis not used elsewhere in this class. Either remove it or reuse it (e.g., inparseAnalysisSettings()) for consistency.If you want to reuse it in
parseAnalysisSettings(), change that method to usethis.objectMapperinstead of instantiating a new one.
527-542: Improve bulk failure diagnosticsThe thrown
ElasticsearchExceptiondoesn’t carryfailedDocuments; the message suggests it does. Consider attaching IDs and reasons to the exception metadata or logging them before throwing to aid troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticsearchConfiguration.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/ElasticCaseRepository.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/ElasticTaskRepository.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ReindexingTask.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticIndexService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/IndexParams.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/Task.java(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ReindexTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java(2 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonDeserializer.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonSerializer.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/event/events/task/IndexTaskEvent.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/event/events/workflow/IndexCaseEvent.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.java(2 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java(0 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java(0 hunks)
💤 Files with no reviewable changes (2)
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java
🧰 Additional context used
🧬 Code Graph Analysis (5)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/DataField.java (1)
Data(10-29)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/FieldWithAllowedNetsField.java (1)
Data(10-26)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.java (1)
NoArgsConstructor(10-33)
application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/IndexParams.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (10)
Data(28-465)Data(116-121)Data(126-149)Data(154-392)Data(302-339)Data(341-353)Data(355-367)Data(375-391)Data(401-447)Data(455-464)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticsearchConfiguration.java (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonDeserializer.java (1)
LocalDateTimeJsonDeserializer(13-30)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonSerializer.java (1)
LocalDateTimeJsonSerializer(11-18)
application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/MessageResource.java (1)
MessageResource(10-31)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonDeserializer.java (1)
LocalDateTimeJsonDeserializer(13-30)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonSerializer.java (1)
LocalDateTimeJsonSerializer(11-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (16)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/CaseField.java (1)
6-7: Import addition is appropriateImporting
java.util.Listis required for the new getter signature.application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/Task.java (1)
111-117: Switched to id-based ProcessResourceId — looks correctUsing
entity.getId()here aligns with the repo-wide migration away fromstringId. No functional issues spotted.nae-object-library/src/main/java/com/netgrif/application/engine/objects/event/events/task/IndexTaskEvent.java (1)
18-19: LGTM: event message now uses canonicalidMessage generation via
task.getId()matches the ID unification done elsewhere.application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
115-116: LGTM: ES query updated toidReplacing
stringIdwithidin the NOT clause is consistent with the new mapping and avoids accidental hits on the current case.nae-object-library/src/main/java/com/netgrif/application/engine/objects/event/events/workflow/IndexCaseEvent.java (1)
19-20: LGTM: switched to id-based messageThe event message now references elasticCase.getId(), aligning with the ID migration across the codebase.
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/DataSearchRequestTest.groovy (1)
151-151: Updated expectation to MIDNIGHT matches new mappingChanging to
LocalTime.MIDNIGHTaligns with the mapping change inElasticCaseMappingService#transformDateField. Looks good.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (1)
115-127: Upsert by id via Optional is consistent with the ID migrationLookup by id, update-or-insert, and logging with the id is aligned with the new approach. Casting to the adapter entity is consistent with the repository type.
application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java (1)
78-81: Setter injection for indexService is fineWires the new IElasticIndexService dependency cleanly and matches existing style in this controller.
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (2)
89-91: Delete by id aligns with the ID migrationSwitching to repository.deleteAllById(caseId) is consistent and correct.
146-147: Mapping search result IDs to workflowService is consistentUsing ElasticCase.getId() here is aligned with repository/entity changes.
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticsearchConfiguration.java (3)
85-96: Custom JsonpMapper wiring looks goodJackson mapper setup with JavaTimeModule and custom LocalDateTime (de)serializers matches the new ES client and date handling strategy.
98-105: Auth precedence is clearCredential precedence over token is explicit and acceptable. No changes needed.
64-68: Confirm host:port format forconnectedTo(...)
ClientConfiguration.connectedTo(...)expects entries in the formhostname:port(nohttp://orhttps://prefix). WhilesanitizeUrls(...)will append yoursearchPortwhen an entry has no “:”, it treats any string containing “:” (for examplehttp://my-es) as already valid. Please verify that in all your deployments:
- The
elasticsearch.urlproperty supplies plain hostnames (optionally with ports), not full URLs.- If you must support full URLs, update
sanitizeUrlsto strip the protocol (e.g.u.replaceFirst("^https?://", "")) before appending the port.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ReindexingTask.java (2)
77-79: Good delegation to the new bulk indexerSwitching the scheduled job to
elasticIndexService.bulkIndex(false, lastRun, null, null)aligns with the new batching flow and centralizes reindexing logic.
91-96: Verify ID semantics in countByIdAndLastModified(...)You pass
aCase.getStringId()to a method renamed tocountByIdAndLastModified(...). If the repository now strictly uses the canonicalid, confirmgetStringId()matches the document ID used in ES. If the domainCaseexposesgetId()as the canonical identifier now, switch to that to avoid false negatives on staleness checks.Would you like me to scan
ElasticCaseRepositoryandCaseto confirm the expected type and source of the ID and propose a patch if needed?application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (1)
328-352: Bulk indexing entrypoint fits the new modelDefaults for batch sizes, safe time windowing (
after.minusMinutes(2)), and counting before streaming are reasonable. Nice.
...e/src/main/java/com/netgrif/application/engine/configuration/ElasticsearchConfiguration.java
Show resolved
Hide resolved
...e/src/main/java/com/netgrif/application/engine/configuration/ElasticsearchConfiguration.java
Show resolved
Hide resolved
...ava/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
Outdated
Show resolved
Hide resolved
...ava/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java
Show resolved
Hide resolved
...ne/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java
Show resolved
Hide resolved
...-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy
Show resolved
Hide resolved
...t-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java
Show resolved
Hide resolved
Replaced usages of `stringId` with `id` across the codebase for consistency. Removed the `stringId` field and updated related methods, constructors, and tests to use `id`. This change improves clarity and aligns the ID handling logic with standard practices.
Replaced the `javax.validation.Valid` import with `jakarta.validation.Valid` to align with the newer Jakarta API standards. This ensures compatibility with updated dependencies and maintains consistency across the project.
Added @ToString.Exclude annotation to `username`, `password`, and `token` fields in `DataConfigurationProperties` to prevent sensitive information from being exposed in the `toString()` method. This ensures better protection of sensitive credentials in log output or debugging scenarios.
Replaced manual iterators with try-with-resources for streamlined resource management using Stream API. Adjusted batch processing logic to improve safety and clarity. This enhances maintainability and reduces resource leakage risks.
Introduced a new `lastRun` parameter in `IndexParams` to specify a starting point based on the `lastModifiedDate` of cases. Updated `ElasticController` to pass this parameter to the `bulkIndex` method, enabling more targeted reindexing operations.
Updated the repository call to use the correct parameter `task.getId()` instead of `task.getTaskId()`. This ensures that tasks are properly identified and removed, preventing potential errors during task deletion.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy (1)
93-97: Fix variable shadowing: netId field remains nullYou assign a local variable instead of the class field, which explains the disabled test complaining about a null id. Assign to the field.
Apply this diff:
- String netId = net.getNet().getStringId() + this.netId = net.getNet().getStringId()application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (1)
160-169: Bug: mutating the wrong instance inside the executor (data race / incorrect test setup)Inside both executor blocks you set the title on the outer
taskinstead of the per-iterationtaskParallel. This introduces shared-state races and invalidates the assertion intent.Apply these fixes:
- task.setTitle(new I18nString("START" + index)) + taskParallel.setTitle(new I18nString("START" + index))and
- task.setTitle(new I18nString("START")) + taskParallel.setTitle(new I18nString("START"))Given this correction, also align the assertion in the second test to the title you actually set:
- assert result.getTitle().getDefaultValue().equals("TestTask"+ index) + assert result.getTitle().getDefaultValue().equals("START")Also applies to: 244-253
🧹 Nitpick comments (13)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy (1)
172-173: Align task identifier with ID migrationThe test still reads taskId from 'stringId'. If tasks now expose 'id', this will fail silently and make subsequent calls break.
Proposed update (verify API payload first):
- taskId = response?._embedded?.tasks?.find { it.title == title }?.stringId + taskId = response?._embedded?.tasks?.find { it.title == title }?.idIf the API still returns 'stringId' for tasks, consider updating the backend representation for consistency with cases, or leave this as-is.
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (1)
159-167: Clarify docs: support multiple hosts/URLs and pluralize name if kept long-termThe field is a List but the Javadoc says “Hostname.” Either adjust the wording to “hosts/URLs” or consider renaming to 'urls' in a future breaking change window.
Apply this doc-only diff:
- /** - * Hostname for the Elasticsearch server. - */ - private List<String> url = List.of("localhost"); + /** + * Hosts/URLs for the Elasticsearch cluster nodes (e.g., ["localhost", "es1:9200", "https://es2:9200"]). + */ + private List<String> url = List.of("localhost");nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (1)
80-82: Avoid generating a random id in no-args constructorConstructors used by mappers/deserializers shouldn’t mutate identity. Generating an ObjectId here can cause unintended inserts or upserts.
Apply this diff:
- public ElasticCase() { - this.id = new ObjectId().toString(); - } + public ElasticCase() { + // Intentionally left blank: id is provided by source Case or deserializer + }application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (3)
194-194: Avoid conflating taskId with id; use a realistic Mongo-like idPer the migration, ElasticTask.id should carry the Mongo Task ObjectId, not the business taskId. Using "TestTask" here can mask mismatches and make reindex behavior non-representative.
Add a stable Mongo-like id in this test and reuse it:
Groovy (outside the selected range, right before setting the id):
def mongoId = new org.bson.types.ObjectId().toHexString()Then update this line:
- task.setId("TestTask") + task.setId(mongoId)
207-207: Keep id consistent with the “Mongo ObjectId” contractFor the parallel task in the same scenario, set the same Mongo-like id so upserts target a single ES document.
- taskParallel.setId("TestTask") + taskParallel.setId(mongoId)Note: mongoId is defined once in the test method as shown in the previous comment.
250-250: Prefer ObjectId-shaped values over synthesized keys for ElasticTask.idIf each parallel task is meant to be a different ES document, generate proper ObjectId-shaped ids to mirror production.
- taskParallel.setId("TestTask"+index) + taskParallel.setId(new org.bson.types.ObjectId().toHexString())application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/ElasticPetriNetRepository.java (1)
19-19: Method name collides with CrudRepository semantics; prefer using built-in deleteByIdSpring Data already provides CrudRepository.deleteById(ID) and deleteAllById(Iterable). Having a custom
deleteAllById(String)is confusing and overlaps with the built-in name that expects an Iterable.Options:
- Remove this method and use the inherited
deleteById(String id).- If you truly need a derived delete query by field, rename to avoid confusion (e.g.,
deleteAllByIdIs(String id)), but for ES doc id the built-indeleteByIdis the right choice.At minimum, update the Javadoc to drop the “string ID” legacy wording:
- /** - * Deletes all {@link ElasticPetriNet} entities with the given string ID. - * - * @param id the string ID of the {@link ElasticPetriNet} entities to delete - */ + /** + * Deletes the {@link ElasticPetriNet} document with the given id (ES _id). + * + * @param id the document id to delete + */ - void deleteAllById(String id); + // Prefer using inherited CrudRepository#deleteById(String id)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java (1)
29-31: Method signature likely not overriding the base-class update; fully qualify and annotateBecause this class name collides with the base type, the parameter here resolves to the adapter type, creating an overload instead of overriding the base-class method. Recommend explicitly overriding the base method to avoid overload confusion and subtle bugs in dynamic dispatch.
Apply this diff:
- public void update(ElasticCase useCase) { - super.update(useCase); - } + @Override + public void update(com.netgrif.application.engine.objects.elastic.domain.ElasticCase useCase) { + super.update(useCase); + }application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java (1)
66-76: Guard the downcast to the adapter type to avoid ClassCastException
repository.save((adapter.ElasticPetriNet) net)assumes callers always pass the adapter subtype. If any caller provides the base type (objects.elastic.domain.ElasticPetriNet), this will throw at runtime.
- Either change the service/interface to accept the adapter subtype and remove the cast, or guard the cast and fail fast with a clear log.
Suggested localized fix:
- Optional<com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticPetriNet> elasticPetriNetOptional = repository.findById(net.getId()); - if (elasticPetriNetOptional.isEmpty()) { - repository.save((com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticPetriNet) net); + Optional<com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticPetriNet> elasticPetriNetOptional = + repository.findById(net.getId()); + if (elasticPetriNetOptional.isEmpty()) { + if (net instanceof com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticPetriNet adapterNet) { + repository.save(adapterNet); + } else { + log.error("Expected adapter ElasticPetriNet, got [{}]. Skipping index.", net.getClass().getName()); + return; + } } else { com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticPetriNet elasticNet = elasticPetriNetOptional.get(); elasticNet.update(net); repository.save(elasticNet); }Optionally, if feasible, update
IElasticPetriNetService#indexto accept the adapter type to enforce this at compile time.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (4)
339-346: Make the 2-minute skew guard configurableThe
after.minusMinutes(2)guard is reasonable to avoid clock skew/race conditions, but hardcoding it reduces flexibility and makes testing harder.Externalize this offset into
DataConfigurationProperties.ElasticsearchProperties.batch.skewGuardSeconds(or similar) and reference it here. If you want, I can draft the config property and wiring.
492-520: Improve failure logging and split message; consider catching IO separatelyThe recursive split approach is fine. The log message “Dividing the requirement.” is unclear. Also,
bulk(...)can throw checked IO-related exceptions depending on the transport.Apply this diff for clearer logs:
- log.warn("Dividing the requirement."); + log.warn("Bulk failed for {} ops; splitting in half and retrying.", operations.size());Optional: add a separate catch for
IOException(if your transport throws it) to provide a more actionable message about connectivity/timeouts.
531-547: Use response.errors() and derive status from failing itemsRelying on
getFirst()may pick a successful item’s status. The client exposesresponse.errors()and per-item statuses. Aggregate accurately and throw with a representative status.Apply this diff:
- private void checkForBulkUpdateFailure(BulkResponse response) { - Map<String, String> failedDocuments = new HashMap<>(); - response.items().forEach(item -> { - if (item.error() != null) { - failedDocuments.put(item.id(), item.error().reason()); - } - }); - - if (!failedDocuments.isEmpty()) { - String message = "Bulk indexing has failures. Use ElasticsearchException.getFailedDocuments() for details [" + failedDocuments.values() + "]"; - throw new ElasticsearchException(message, - ErrorResponse.of(builder -> builder - .error(ErrorCause.of(errorCauseBuilder -> errorCauseBuilder.reason(message))) - .status(response.items().getFirst().status()))); - } - } + private void checkForBulkUpdateFailure(BulkResponse response) { + if (!response.errors()) { + return; + } + + Map<String, String> failedDocuments = new LinkedHashMap<>(); + response.items().forEach(item -> { + if (item.error() != null) { + failedDocuments.put(item.id(), item.error().reason()); + } + }); + + if (!failedDocuments.isEmpty()) { + String message = "Bulk indexing has failures: " + failedDocuments; + int status = response.items().stream() + .filter(i -> i.error() != null) + .map(i -> i.status() == null ? 500 : i.status()) + .findFirst() + .orElse(500); + throw new ElasticsearchException(message, + ErrorResponse.of(builder -> builder + .error(ErrorCause.of(ecb -> ecb.reason(message))) + .status(status))); + } + }
621-627: Configured ObjectMapper is unused; either wire into the ES client or removeYou create and configure an
ObjectMapper(with JavaTimeModule and custom LocalDateTime serializers), but never use it. The Elasticsearch Java client uses its own mapper. If you intend to control date serialization (e.g., yyyy-MM-dd'T'HH:mm:ss.SSS), you must provide this mapper when building the client transport (JacksonJsonpMapper).
- If ES client is built elsewhere (e.g., in a configuration class), inject a preconfigured
ElasticsearchClientthat uses:
- new RestClientTransport(restClient, new JacksonJsonpMapper(objectMapper))
- Otherwise, remove the field and method to avoid confusion.
Would you like me to draft the configuration snippet to build the client with the custom mapper?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(6 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/ElasticPetriNetRepository.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/IndexParams.java(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy(3 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/insurance/mvc/InsuranceTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java(2 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java
- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
- application-engine/src/main/java/com/netgrif/application/engine/elastic/web/requestbodies/IndexParams.java
- application-engine/src/main/java/com/netgrif/application/engine/elastic/web/ElasticController.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-08-19T20:07:15.600Z
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/ElasticPetriNetRepository.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovynae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
PR: netgrif/application-engine#331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
PR: netgrif/application-engine#334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-08-19T20:13:40.040Z
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.040Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java
🧬 Code Graph Analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (4)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonDeserializer.java (1)
LocalDateTimeJsonDeserializer(13-30)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/serializer/LocalDateTimeJsonSerializer.java (1)
LocalDateTimeJsonSerializer(11-18)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
Slf4j(28-327)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/ElasticsearchRunner.java (1)
Slf4j(16-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (12)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (2)
277-279: Good addition: Batch sizes with validation for bulk indexingIntroducing BatchProperties with @min validations is a solid foundation for tunable, safe batching.
Also applies to: 373-395
174-176: KeepsearchPort—it’s still in use
ThesearchPortproperty is referenced in several places and shouldn’t be removed:
- DashboardService.java (line 21): used to build the Elasticsearch query URL
- ElasticsearchConfiguration.java (line 108): used in
sanitizeUrlsto append a default port when none is provided- application.yaml (line 19): defines the default value
Since these usages rely on
searchPort, it must remain.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (2)
91-93: LGTM: Truncate creationDate to millisecondsThis aligns with ES date precision and avoids noisy diffs.
84-86: Case#getId() is undefined in the workflow domain Case classThe
Caseclass (nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Case.java) only providesgetStringId()and has nogetId()method. Applying the suggested change will break compilation. Before replacing any calls togetStringId(), add agetId()alias in theCaseclass, for example:// in Case.java public String getId() { return getStringId(); }Likely an incorrect or invalid review comment.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java (1)
22-24: Adding an explicit no-arg constructor is correct hereSubclasses that declare other constructors won’t get a synthetic no-arg constructor; this ensures Spring Data/serializers can instantiate the class. LGTM.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
41-43: Mapping id from PetriNet.stringId — LGTMAssigning
this.id = net.getStringId()matches the migration to use the Mongo id as the ES doc id.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java (1)
21-23: No-args constructor is a good addition for Spring Data/JacksonExplicit default constructor helps frameworks and avoids Lombok coupling. Looks good.
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java (3)
78-82: Duplicate handling path is soundCatching
InvalidDataAccessApiUsageException, deleting by ID, and re-saving is a pragmatic recovery for duplicate index state. Logging also references the new ID model consistently.
94-96: Removal by ID aligns with the ID migrationUsing
deleteAllById(id)matches the standardized ID usage. LGTM.
124-125: Search mapping: switched to ID correctlyCollecting PetriNet IDs via
ElasticPetriNet::getIdbefore fetching domain objects is consistent with the migration away from stringId.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (2)
399-406: Task batching page math and streaming look goodUses ceiling division and try-with-resources on the Mongo stream. Matches the fixes applied to case batching.
370-379: No ID mismatch — keep aCase.getStringId()Task.caseId is a String that holds the Case string id (ProcessResourceId full id), so using aCase.getStringId() to query tasks is correct. Evidence:
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java — private String caseId; (getter/setter present).
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/ProcessResourceId.java — toString()/getFullId() returns the composite "-" used by Case.getStringId().
- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java — collects caseIds.add(aCase.getStringId()) and queries Tasks with Criteria.where("caseId").in(caseIds).
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java — maps this.caseId = task.getCaseId().
- Multiple call sites use Case.getStringId() when querying tasks (e.g. FilterImportExportService, MenuImportExportService, TaskRepository.findAllByCaseId / findByCaseIdIn).
Conclusion: the original concern is incorrect — no change required.
Likely an incorrect or invalid review comment.
...ava/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java
Show resolved
Hide resolved
.../src/test/groovy/com/netgrif/application/engine/auth/WorkflowAuthorizationServiceTest.groovy
Outdated
Show resolved
Hide resolved
...ion-engine/src/test/groovy/com/netgrif/application/engine/insurance/mvc/InsuranceTest.groovy
Outdated
Show resolved
Hide resolved
.../src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy
Outdated
Show resolved
Hide resolved
...e/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
Outdated
Show resolved
Hide resolved
...ary/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java
Outdated
Show resolved
Hide resolved
Previously, the task removal process logged and deleted tasks using inconsistent ID references (`getId` vs. `getTaskId`). This change ensures consistent usage of `getTaskId` for both logging and deletion, preventing potential errors and improving clarity.
Replaced the deprecated `getId` method with direct `.id` field access for better readability and consistency. This update ensures compatibility with the current data structure and cleans up test code syntax.
Removed explicit no-argument constructors across Elastic* domain objects and replaced them with the @NoArgsConstructor annotation for cleaner and more consistent code. Additionally, adjusted test cases to properly initialize task IDs where required and updated instance checks to reflect repository changes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy (3)
90-97: Fix variable shadowing that leavesnetIdfield null (cause of the Disabled test)In
before(), the localString netId = ...shadows the class fieldnetId. The class field remains null, which leads to an invalid payload (netId: null) and the IllegalArgumentException noted in the @disabled reason.Apply this diff to set the class field and use it consistently:
- String netId = net.getNet().getStringId() + this.netId = net.getNet().getStringId() - def auths = importHelper.createAuthorities(["user": Authority.user, "admin": Authority.admin]) - def processRoles = userProcessRoleRepository.findAllByProcessId(netId) + def auths = importHelper.createAuthorities(["user": Authority.user, "admin": Authority.admin]) + def processRoles = userProcessRoleRepository.findAllByProcessId(this.netId)After this change, consider removing the @disabled annotation.
171-173: Update test assertions to useidinstead of removedstringIdThe migration removed
stringIdfrom task payloads—tests must be updated to use.id. We found two occurrences:
- application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy:172
- application-engine/src/test/groovy/com/netgrif/application/engine/insurance/mvc/InsuranceTest.groovy:287
Replace in both files:
- taskId = response?._embedded?.tasks?.find { it.title == title }?.stringId + taskId = response?._embedded?.tasks?.find { it.title == title }?.idAfter updating, scan for any remaining
stringIdusages in tests:rg -nP --type groovy '\._embedded\?.tasks\?.*stringId'
141-147: Replace deprecated MediaType.APPLICATION_JSON_UTF8_VALUE with APPLICATION_JSON_VALUESpring Framework 6 (Spring Boot 3.x) no longer provides
MediaType.APPLICATION_JSON_UTF8_VALUE. Update all occurrences inProcessRoleTest.groovyto useMediaType.APPLICATION_JSON_VALUEso the tests compile.Locations:
- application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy
• Lines 141–147
• Lines 161–167Apply this diff in both places:
- .contentType(MediaType.APPLICATION_JSON_UTF8_VALUE) + .contentType(MediaType.APPLICATION_JSON_VALUE)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (3)
165-173: Bug: Mutating the wrong instance inside parallel section (taskvstaskParallel)Within the worker, you instantiate
taskParallelbut then set fields on the outertask. You subsequently schedule indexing fortaskParallel, which is missingid,taskId, andtitle. This breaks isolation and makes the test nondeterministic.Apply this diff:
- ElasticTask taskParallel = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask() - task.setId("TestTask" + index) - task.setTaskId("TestTask" + index) - task.setTitle(new I18nString("START" + index)) - taskParallel.setProcessId("TestProcess") - Future<ElasticTask> resultFuture = elasticTaskService.scheduleTaskIndexing(taskParallel) + ElasticTask taskParallel = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask() + taskParallel.setId("TestTask" + index) + taskParallel.setTaskId("TestTask" + index) + taskParallel.setTitle(new I18nString("START" + index)) + taskParallel.setProcessId("TestProcess") + Future<ElasticTask> resultFuture = elasticTaskService.scheduleTaskIndexing(taskParallel)
207-215: Same bug: set fields ontaskParallel, nottaskRepeat of the above issue in
reindexTaskTest().Apply this diff:
- ElasticTask taskParallel = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask() - task.setId("TestTask" + index) - task.setTaskId("TestTask" + index) - task.setTitle(new I18nString("START" + index)) - Future<ElasticTask> resultFuture = elasticTaskService.scheduleTaskIndexing(taskParallel) + ElasticTask taskParallel = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask() + taskParallel.setId("TestTask" + index) + taskParallel.setTaskId("TestTask" + index) + taskParallel.setTitle(new I18nString("START" + index)) + Future<ElasticTask> resultFuture = elasticTaskService.scheduleTaskIndexing(taskParallel)
250-257: Two issues: wrong instance mutated and inconsistent assertion
- Fields are set on
taskinstead oftaskParallel.- The assertion expects title
"TestTask${index}"but you set title to"START".Apply this diff:
- ElasticTask taskParallel = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask() - task.setId("TestTask" + index) - task.setTaskId("TestTask" + index) - task.setTitle(new I18nString("START")) - Future<ElasticTask> resultFuture = elasticTaskService.scheduleTaskIndexing(taskParallel) + ElasticTask taskParallel = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask() + taskParallel.setId("TestTask" + index) + taskParallel.setTaskId("TestTask" + index) + taskParallel.setTitle(new I18nString("START")) + Future<ElasticTask> resultFuture = elasticTaskService.scheduleTaskIndexing(taskParallel)And either fix the expectation to match title:
- assert result.getTitle().getDefaultValue().equals("TestTask"+ index) + assert result.getTitle().getDefaultValue().equals("START")Or, if the intent was to assert identity, prefer checking taskId:
- assert result.getTitle().getDefaultValue().equals("TestTask"+ index) + assert result.getTaskId().equals("TestTask" + index)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (1)
109-110: Prevent NPE when incrementingversionin ElasticCase.updateThe
versionfield is declared asLongand isn’t initialized in the constructor, soversion++will throw a NullPointerException on the first call.• File:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
Line 109, replace:version++;with:
version = (version == null) ? 1L : version + 1L;• Optional: if
versionis meant to mirror your domainCaseversion rather than act as an index-local counter, initialize it in theElasticCase(Case useCase)constructor (e.g.this.version = useCase.getVersion()) and adjust your increment logic to reflect domain semantics.
♻️ Duplicate comments (1)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (1)
134-134: Post-delete ES check corrected to assert absence — LGTMThe assertion now correctly verifies that the ES document is absent after delete. This addresses the earlier inversion and aligns with the Optional-based repository API.
🧹 Nitpick comments (4)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (3)
132-136: Replace fixed sleep with polling to de-flake ES eventual consistencyFixed sleeps are brittle and slow. Poll for the ES condition with a short timeout to make the test more reliable and faster.
Apply this diff:
petriNetService.deletePetriNet(testNet.stringId, superCreator.getLoggedSuper()) assert petriNetRepository.count() == processCount - Thread.sleep(5000) - assert elasticPetriNetRepository.findById(testNet.stringId).isEmpty() + awaitTrue("Elastic doc deleted") { elasticPetriNetRepository.findById(testNet.stringId).isEmpty() }Add this helper method inside the test class (e.g., near the bottom):
private static void awaitTrue(String description, Closure<Boolean> condition, long timeoutMs = 10_000L, long pollMs = 100L) { long deadline = System.currentTimeMillis() + timeoutMs while (System.currentTimeMillis() < deadline) { if (condition.call()) return Thread.sleep(pollMs) } assert false : "Timed out waiting for condition: ${description}" }
142-149: Prefer Assertions.assertThrows over manual try/catchCleaner, asserts the exception type, and keeps the test focused.
- boolean exceptionThrown = false - try { - petriNetService.get(new ObjectId(testNet.stringId)) - } catch (IllegalArgumentException e) { - exceptionThrown = true - assert e.message.contains(testNet.stringId) - } - assert exceptionThrown + def ex = org.junit.jupiter.api.Assertions.assertThrows(IllegalArgumentException) { + petriNetService.get(new ObjectId(testNet.stringId)) + } + assert ex.message?.contains(testNet.stringId)
42-42: Provide a reason for @disabled to aid future re-enablementAttaching context (e.g., ticket ID, flakiness reason) helps maintainers know when/how to re-enable.
-@Disabled +@Disabled("Flaky due to ES eventual consistency; re-enable after delete propagation is deterministic (NAE-2168)")application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (1)
159-181: Shut down ExecutorService to avoid thread leakage in testsThe fixed thread pool isn't shut down, which can leak threads and affect subsequent tests.
Apply this pattern at the end of each test using
executorService:latch.await(5, TimeUnit.SECONDS) + executorService.shutdownNow()And in the test with 50s await:
latch.await(50, TimeUnit.SECONDS) + executorService.shutdownNow()Also applies to: 201-223, 243-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy(8 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/insurance/mvc/InsuranceTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java(2 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java(0 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java(0 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java(0 hunks)
💤 Files with no reviewable changes (3)
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java
🚧 Files skipped from review as they are similar to previous changes (3)
- application-engine/src/test/groovy/com/netgrif/application/engine/insurance/mvc/InsuranceTest.groovy
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
PR: netgrif/application-engine#334
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java:204-214
Timestamp: 2025-07-31T23:40:46.499Z
Learning: In the PetriNetService.importPetriNet method, existingNet.getVersion() cannot be null because all existing nets in the system were deployed through processes that ensure every net always has a version assigned.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy
📚 Learning: 2025-08-19T20:07:15.600Z
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovynae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (4)
application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy (1)
114-131: Confirmed — keep using stringId (no test change required)Search shows the project still exposes and relies on stringId as the PetriNet identifier and the Elastic mapping is populated from it, so the test's use of testNet.stringId is correct.
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java — public String getStringId() (returns _id.toString())
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java — constructor sets this.id = net.getStringId()
- nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java — @id on getId() which returns super.getId() (mapped to the same value)
- application-engine/src/test/groovy/com/netgrif/application/engine/petrinet/service/PetriNetServiceTest.groovy — testNet.stringId usage is consistent with current domain API (many other tests still reference .stringId)
No changes needed to the test unless the domain API is intentionally changed in a future commit to use a different identifier (e.g., migrating callers to getId()).
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (1)
96-99: LGTM on migrating tosetId(...)The switch from
setStringId(...)tosetId(...)aligns with the new identity model and keeps the tests consistent with the updated ElasticTask API.Also applies to: 130-134, 153-157, 196-199, 239-242
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (2)
82-82: LGTM on canonicalizing toid = useCase.getStringId()This is consistent with the ID consolidation (single
idfield representing the Case's Mongo/Object ID).
89-89: Good: truncatecreationDateto millisecondsAligns with ES date precision and avoids mapper round-trip drift.
.../src/test/groovy/com/netgrif/application/engine/petrinet/domain/roles/ProcessRoleTest.groovy
Show resolved
Hide resolved
Updated the null check and logging to use the correct method `getId()` instead of `getTaskId()`. This ensures proper validation and accurate debug logs for scheduled tasks.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (1)
88-90: Do not clear the active flag before scheduling next; also use the same id key for queue operationsClearing activeTasks before polling the next task allows a racing scheduleOperation to spin up a second worker for the same key. Let scheduleNextTask clear the flag only when the queue is empty, and pass the same key used during enqueueing.
Apply this diff:
- activeTasks.remove(task.getTaskId()); - scheduleNextTask(task.getTaskId()); + // Keep active flag until the queue drains; scheduleNextTask clears it when empty. + scheduleNextTask(task.getTask().getId());
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (1)
140-144: Delete by canonical document id (task.getId), not taskId; align with repository API and PR migration to idThis repeats a previously raised concern. With id now being the canonical unique identifier, delete by id to avoid accidental multi-doc deletes and to be consistent with indexTaskWorker’s duplicate cleanup.
Apply this diff:
- log.debug("Remove task [{}] in thread [{}]", task.getTaskId(), Thread.currentThread().getName()); + log.debug("Remove task [{}] in thread [{}]", task.getId(), Thread.currentThread().getName()); @@ - log.debug("[{}]: Task \"{}\" [{}] removed", task.getCaseId(), task.getTitle(), task.getTaskId()); - return repository.deleteAllByTaskId(task.getTaskId()); + log.debug("[{}]: Task \"{}\" [{}] removed", task.getCaseId(), task.getTitle(), task.getId()); + repository.deleteAllById(task.getId()); + return task;
🧹 Nitpick comments (2)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (2)
55-61: Log the actual exception and avoid string concatenation in logsUse parameterized logging and pass the throwable so stack traces are preserved.
Apply this diff:
- } catch (Exception e) { - log.error("Queue error:" + e.getMessage()); - throw e; - } + } catch (Exception e) { + log.error("Queue error while scheduling operation for task {}", taskId, e); + throw e; + }
72-91: Unify logging key in processTask and avoid mixing id sourcesOptional refactor: derive a single key once and use it for logs and queue lifecycle to reduce drift between id and taskId.
Example:
private ElasticTask processTask(ElasticTaskJob task) { final String key = task.getTask().getId(); try { log.debug("Processing task: {}", key); switch (task.getJobType()) { case INDEX: return indexTaskWorker(task.getTask()); case REMOVE: return removeTaskWorker(task.getTask()); default: log.warn("Unknown job type for task: {}", key); throw new IllegalArgumentException("Unknown job type: " + task.getJobType()); } } catch (Exception e) { log.error("Error processing task {}: {}", key, e.getMessage(), e); throw e; } finally { // see fix on Lines 88-90 to avoid clearing active prematurely scheduleNextTask(key); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (1)
16-16: LGTM: Optional import aligns with repository.findById usageThe move to Optional improves null-safety and reads better with the new repository API.
...ne/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
Outdated
Show resolved
Hide resolved
...ne/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
Outdated
Show resolved
Hide resolved
Replace redundant `getTask().getId()` calls with `getTaskId()` for clarity and simplify task identification. Update related test cases to reflect changes and ensure consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (1)
251-257: Fix variable usage and assertion mismatch in parallel test.
- Same bug: you mutate
taskinstead oftaskParallel.- Title is set to "START" (no index), but the assertion expects "TestTask" + index. Align these.
- task.setId("TestTask" + index) - task.setTaskId("TestTask" + index) - task.setTitle(new I18nString("START")) + taskParallel.setId("TestTask" + index) + taskParallel.setTaskId("TestTask" + index) + taskParallel.setTitle(new I18nString("START" + index)) Future<ElasticTask> resultFuture = elasticTaskService.scheduleTaskIndexing(taskParallel) ElasticTask result = resultFuture.get() assert result != null - assert result.getTitle().getDefaultValue().equals("TestTask"+ index) + assert result.getTitle().getDefaultValue().equals("START" + index)
🧹 Nitpick comments (3)
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (3)
95-103: Use a single ElasticTask type consistently (adapter vs. objects).You’re constructing
com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTaskwhile the importedElasticTaskrefers tocom.netgrif.application.engine.objects.elastic.domain.ElasticTask. This is confusing and risks type mismatches. Prefer aligning the declared type with the constructed one (apply similarly in other tests).Apply this change locally in this spot (and mirror it in the other test methods that instantiate the adapter class):
-ElasticTask task = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask() +com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask task = new com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticTask()Note: The move from stringId to id is correct per the migration; just ensure all API points expect/return the adapter type where used.
139-146: CountDownLatch is not wired; this is a timed sleep and may be flaky.
latchis never decremented, soawaitjust waits for a timeout. Prefer polling the repository or awaiting a completion signal from the service to reduce flakiness.Example replacement for these lines:
-CountDownLatch latch = new CountDownLatch(1) -elasticTaskService.removeByPetriNetId("Process") -latch.await(10, TimeUnit.SECONDS) -ElasticTask deletedTask = elasticTaskRepository.findByTaskId("TestTask1") +elasticTaskService.removeByPetriNetId("Process") +long deadline = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(10) +while (System.currentTimeMillis() < deadline) { + if (elasticTaskRepository.findByTaskId("TestTask1") == null) { + break + } + Thread.sleep(50) +} +ElasticTask deletedTask = elasticTaskRepository.findByTaskId("TestTask1")
239-241: Nit: consider setting taskId alongside id for consistency across tests.Not strictly required here, but keeping both identifiers initialized uniformly avoids surprises if downstream logic queries by
taskId.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java(4 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-08-19T20:07:15.600Z
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
📚 Learning: 2025-08-20T07:27:02.645Z
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.645Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (3)
application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy (3)
130-132: LGTM: id/taskId initialization is consistent with the migration.Initializing both
idandtaskIdto the same value aligns with the new identity model in tests.
153-155: LGTM: base record’s id initialization is aligned with id-based identity.
196-199: LGTM: id initialization before reindexing is fine.
...ication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
Outdated
Show resolved
Hide resolved
...ication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
Outdated
Show resolved
Hide resolved
Enhanced logging to include task ID during indexing after duplicate cleanup. Corrected variable assignments and made minor adjustments in test cases to fix and align task ID consistency for better clarity and reliability.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (2)
133-136: Don’t swallow runtime failures — propagate so futures complete exceptionallyCatching RuntimeException and returning null masks failures. Let processTask handle the error and complete the Future exceptionally.
- } catch (RuntimeException e) { - log.error("Elastic executor was killed before finish: {}", e.getMessage()); - } + } catch (RuntimeException e) { + log.error("Elastic executor was killed before finish: {}", e.getMessage(), e); + throw e; + }
139-147: Removal worker should also propagate runtime failuresSame rationale as index path: don’t mask remove errors by returning task.
- } catch (RuntimeException e) { - log.error("Elastic executor was killed before finish: {}", e.getMessage()); - } - return task; + } catch (RuntimeException e) { + log.error("Elastic executor was killed before finish: {}", e.getMessage(), e); + throw e; + }Note: processTask will catch and rethrow, and the caller future will complete exceptionally as expected.
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (1)
142-143: Confirm delete key: id vs taskId (prior discussion) and align logs accordinglyGiven the PR-wide migration to id and the earlier resolution that task IDs are globally unique now, please confirm whether removal should target repository.deleteAllById(task.getId()) instead of deleteAllByTaskId(task.getTaskId()). The current log also references taskId.
If the canonical identifier is id, consider:
- log.debug("[{}]: Task \"{}\" [{}] removed", task.getCaseId(), task.getTitle(), task.getTaskId()); - return repository.deleteAllByTaskId(task.getTaskId()); + log.debug("[{}]: Task \"{}\" [{}] removed", task.getCaseId(), task.getTitle(), task.getId()); + return repository.deleteAllById(task.getId());To verify repository capabilities and current usages:
#!/bin/bash # Show ElasticTaskRepository signatures for delete methods rg -nP -C3 'interface\s+ElasticTaskRepository\b|deleteAllBy(TaskId|Id)\s*\(' --type=java # Call sites of deleteAllByTaskId vs deleteAllById rg -nP -C3 '\bdeleteAllByTaskId\s*\(' --type=java rg -nP -C3 '\bdeleteAllById\s*\(' --type=java
🧹 Nitpick comments (5)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (5)
55-61: Improve error logging to include stacktraceConcatenating the message drops the stacktrace. Log the exception object.
Apply:
- log.error("Queue error:" + e.getMessage()); + log.error("Queue error: {}", e.getMessage(), e);
115-117: Align start-of-index log with canonical idOther logs in this method use task.getId(); use it here too for traceability.
- log.debug("Indexing task [{}] in thread [{}]", task.getTaskId(), Thread.currentThread().getName()); + log.debug("Indexing task [{}] in thread [{}]", task.getId(), Thread.currentThread().getName());
150-176: Prefer using scheduleOperation() for removals to preserve single-worker-per-key semanticsBy calling processTask(job) directly via supplyAsync, you bypass the per-task queue and activeTasks gating. Using scheduleOperation keeps sequencing guarantees consistent with INDEX jobs.
- CompletableFuture<ElasticTask> removeJobFuture = CompletableFuture.supplyAsync(() -> processTask(job), elasticTaskExecutor); + Future<ElasticTask> removeJobFuture = scheduleOperation(job);The timeout loop with removeJobFuture.get(waitTime, TimeUnit.SECONDS) can stay unchanged.
109-113: Minor: drop throws InterruptedException and consider graceful shutdown waitThe method doesn’t throw, and you might want to await termination if shutdown ordering matters at shutdown.
- public void destroy() throws InterruptedException { + public void destroy() { log.info("Shutting down ElasticTaskQueueManager"); elasticTaskExecutor.shutdown(); }Optional: if you need to wait for completion, consider awaiting the underlying pool termination.
88-104: Redundant activeTasks.remove in scheduleNextTask()’s else branchactiveTasks is already removed in processTask’s finally before calling scheduleNextTask. The extra removal is redundant but harmless.
- } else { - activeTasks.remove(taskId); - taskQueues.remove(taskId); - } + } else { + taskQueues.remove(taskId); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java(4 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.600Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-08-20T07:27:02.645Z
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.645Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
📚 Learning: 2025-08-20T07:24:22.531Z
Learnt from: renczesstefan
PR: netgrif/application-engine#339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:24:22.531Z
Learning: When renczesstefan indicates a conversation is resolved after conflicting guidance, respect their decision and don't push for further changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (4)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java (4)
16-16: LGTM: Optional import matches new repository API usageAdding Optional is appropriate for repository.findById() usage below.
119-127: Use of Optional and update/insert flow looks correctfindById/update/save path is sound and aligns with the id-centric repository API.
128-133: Duplicate-cleanup path now assigns saved entity — good; keep id-centric loggingNice fix assigning repository.save(...) to elasticTask and logging with id. This prevents returning null and keeps logs consistent.
38-43: Verified: Business taskId queue keying is correct and repository methods align
- scheduleOperation uses
task.getTaskId()as the queue key- removeTaskWorker calls
repository.deleteAllByTaskId(task.getTaskId())for cleanup- indexTaskWorker uses
repository.findById(task.getId())/deleteAllById(task.getId())on the document IDElasticTaskRepositorydeclares exactly these methods (findByTaskId,deleteAllByTaskId,findById,deleteAllById) and no conflicting usages existAll checks pass—no changes needed.
Description
Updated bulk index with new Elasticsearch client configuration. Updated ElasticCase and ElasticTask ID.
Implements NAE-2168
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor