[NAE-2267] For some mongo collections there are no indexes #386
Conversation
Introduced a new @indexable annotation to flag classes for MongoDB indexing. Updated Task, PetriNet, and Case classes to use @indexable and adjusted the Mongo index configurator to process this annotation. This simplifies index configuration and enhances maintainability.
WalkthroughThis PR restructures Redis session configuration to support Sentinel and nested properties, refactors Elasticsearch operations to use queue-based bulk processing, adds I18nString-based translation handling for field mappings, introduces indexing annotations for domain entities, updates parent POM version across modules, and changes the base Docker image from OpenJDK to Eclipse Temurin. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ElasticCaseService
participant Queue as ElasticQueueManager
participant ES as Elasticsearch
participant Publisher as EventPublisher
Client->>ElasticCaseService: index(case)
ElasticCaseService->>ElasticCaseService: createIndexOperation()
ElasticCaseService->>Queue: push(BulkOperationWrapper)
Queue->>Queue: enqueue operation
rect rgb(200, 240, 200)
Note over Queue: Timer triggered or<br/>max batch reached
end
Queue->>ES: bulk([operations])
ES-->>Queue: success with UUID
Queue->>Publisher: publishEvent() for each op
Publisher-->>Client: event emitted
Client->>ElasticCaseService: remove(caseId)
ElasticCaseService->>Queue: push(delete BulkOperationWrapper)
Queue->>Queue: enqueue operation
rect rgb(200, 240, 200)
Note over Queue: Periodic flush or<br/>batch full
end
Queue->>ES: bulk([delete ops])
ES-->>Queue: success
sequenceDiagram
autonumber
participant Config as SessionConfiguration
participant Redis as Redis<br/>Sentinel/Standalone
Config->>Config: jedisConnectionFactory()
alt Sentinel Master Configured
Config->>Config: redisSentinelConfiguration()
rect rgb(200, 220, 240)
Note over Config: Parse sentinel nodes<br/>Set master + password
end
Config->>Redis: JedisConnectionFactory<br/>(Sentinel config)
else Standalone Mode
Config->>Config: standaloneRedisConfiguration()
Config->>Redis: JedisConnectionFactory<br/>(Standalone config)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/ImpersonationConfigurationProperties.java (1)
18-21: Update the documentation to reflect the new SpEL expression.The documentation comment still references
redisProperties.namespace, but the actual implementation now usesredisProperties.session.namespace.Apply this diff to update the documentation:
/** * The Redis namespace used for storing impersonation configurations. - * This value is resolved from the "redisProperties.namespace" SpEL expression. + * This value is resolved from the "redisProperties.session.namespace" SpEL expression. */Dockerfile (1)
1-17: Consider adding HEALTHCHECK and USER directives for production hardening.While not blocking, these improvements align with container security best practices:
- Add
HEALTHCHECKinstruction to enable container orchestration platforms to monitor application health- Add
USERdirective to run as non-root for improved security postureExample:
+RUN addgroup --system netgrif && adduser --system --ingroup netgrif netgrif +RUN chown -R netgrif:netgrif /opt/netgrif/engine + WORKDIR /opt/netgrif/engine EXPOSE 8080 +HEALTHCHECK --interval=30s --timeout=3s --start-period=40s --retries=3 \ + CMD curl -f http://localhost:8080/actuator/health || exit 1 + +USER netgrif ENTRYPOINT ["java","-Dfile.encoding=UTF-8","-jar","app.jar"]application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
2419-2427: Add null/empty check beforenew ObjectId(role.processId)to prevent runtime exceptionThe code at line 2424 calls
new ObjectId(role.processId)without validation. However, theProcessRoleclass constructor explicitly handles nullprocessId(line 55 in nae-object-library), proving the field can be null. If a non-global role has null or emptyprocessId, this will throwIllegalArgumentExceptionorNullPointerExceptionat runtime.Add a defensive check similar to the pattern in
Task.java(lines 172–185):if (role.processId != null && !role.processId.isEmpty()) { PetriNet net = petriNetService.get(new ObjectId(role.processId)) // use net... } else { // handle missing processId case (skip role or use fallback) }nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java (1)
16-38: I18nString transition looks consistent; consider not exposing the internal mutable map directlyThe switch of constructors to
Map.Entry<String, I18nString>aligns with the rest of the I18nString‑based mapping and looks correct.
getKeyValueTranslations()currently exposessuper.keyValueTranslationsdirectly, which allows callers to mutate the internal state of the superclass:@Field(type = Flattened, index = false) public Map<String, I18nString> getKeyValueTranslations() { return super.keyValueTranslations; }If external mutation is not desired, consider returning an unmodifiable view:
@Field(type = Flattened, index = false) public Map<String, I18nString> getKeyValueTranslations() { - return super.keyValueTranslations; + return java.util.Collections.unmodifiableMap(super.keyValueTranslations); }Otherwise, current mapping annotations look fine.
♻️ Duplicate comments (3)
nae-user-ce/pom.xml (1)
9-9: Version downgrade from RC9 to RC8.2.This module also exhibits the parent version downgrade from
7.0.0-RC9to7.0.0-RC8.2. Please see the verification comment onnae-object-library/pom.xmlfor the consolidated analysis of this version change across all modules.pom.xml (1)
9-9: Root parent POM version downgraded from RC9 to RC8.2.This is the parent POM that propagates the version to all child modules. The version downgrade requires verification as noted in the
nae-object-library/pom.xmlreview comment.application-engine/pom.xml (1)
9-9: Version downgrade from RC9 to RC8.2.This module also exhibits the parent version downgrade. Please see the verification comment on
nae-object-library/pom.xmlfor the consolidated analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (36)
Dockerfile(1 hunks)application-engine/pom.xml(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/AbstractSecurityConfiguration.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(9 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/ImpersonationConfigurationProperties.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/BulkOperationWrapper.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticCaseService.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/manager/service/SessionManagerService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseEventHandler.java(1 hunks)application-engine/src/main/resources/application.yaml(1 hunks)application-engine/src/test/resources/application-test.yaml(1 hunks)nae-object-library/pom.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/annotations/Indexable.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java(2 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nStringUtils.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Case.java(4 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java(2 hunks)nae-spring-core-adapter/pom.xml(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/AbstractMongoIndexesConfigurator.java(2 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)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java(2 hunks)nae-user-ce/pom.xml(1 hunks)nae-user-common/pom.xml(1 hunks)pom.xml(1 hunks)
💤 Files with no reviewable changes (1)
- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticCaseService.java
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/AbstractMongoIndexesConfigurator.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.javaapplication-engine/src/main/resources/application.yamlnae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.javaapplication-engine/src/test/resources/application-test.yamlnae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
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:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.javaapplication-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseEventHandler.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-07-31T23:40:46.499Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 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:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java
📚 Learning: 2025-09-29T10:31:57.325Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 362
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/ProcessRoleService.java:513-529
Timestamp: 2025-09-29T10:31:57.325Z
Learning: PetriNet.getStringId() returns a simple ObjectId string representation (_id.toString()), not a composite Netgrif ID format, so new ObjectId(petriNetId) works correctly when petriNetId comes from PetriNet.getStringId().
Applied to files:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 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.087Z
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.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Case.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java
📚 Learning: 2025-11-10T15:28:13.391Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 377
File: application-engine/src/main/resources/application.yaml:26-27
Timestamp: 2025-11-10T15:28:13.391Z
Learning: In the netgrif/application-engine project, all engine deployments within the same cluster intentionally share the same Redis session namespace. The change from database-specific namespace to a shared "netgrif" default (configurable via NETGRIF_ENGINE_DATA_REDIS_SESSION_NAMESPACE) is by design to enable session sharing across deployments.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.javaapplication-engine/src/main/resources/application.yamlapplication-engine/src/main/java/com/netgrif/application/engine/configuration/AbstractSecurityConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/ImpersonationConfigurationProperties.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/manager/service/SessionManagerService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
📚 Learning: 2025-11-07T13:11:20.622Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.622Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
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:
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 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:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java
🧬 Code graph analysis (7)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/annotations/Indexable.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (1)
Indexable(29-441)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nStringUtils.java (1)
I18nStringUtils(8-18)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Case.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (1)
Indexable(29-441)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (1)
Indexable(29-441)
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java (2)
application-engine/src/main/java/com/netgrif/application/engine/manager/service/SessionManagerService.java (1)
Slf4j(20-71)application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java (1)
Configuration(11-24)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/AbstractMongoIndexesConfigurator.java (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (1)
Indexable(29-441)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java (4)
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/I18nField.java (1)
Data(11-34)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/MapField.java (1)
NoArgsConstructor(13-39)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/I18nStringUtils.java (1)
I18nStringUtils(8-18)
🪛 Checkov (3.2.334)
Dockerfile
[low] 1-17: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-17: Ensure that a user for the container has been created
(CKV_DOCKER_3)
⏰ 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). (5)
- 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 (27)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticTask.java (1)
11-26: Serializable integration looks fine; confirm serializability of all fields and subclassesMaking
ElasticTaskimplementSerializablewith an explicitserialVersionUIDand@Serialis correct and consistent with Java 21 usage. The only concern is runtime: all non‑transient fields (e.g.,I18nString, and any additional fields in concrete subclasses) must themselves beSerializableor handled specially, otherwise Java serialization will fail withNotSerializableException.Please verify:
I18nString(and any other custom types reachable from these fields) implementsSerializable, or- You intend to never use Java serialization over these specific fields, in which case consider marking non‑serializable ones as
transient.If subclasses introduce new fields, they should be checked for the same constraint and may warrant their own
serialVersionUIDif long‑term compatibility is required.application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/ImpersonationConfigurationProperties.java (1)
22-23: Nested property path is correctly configured—no changes needed.Verification confirms the SpEL expression
#{redisProperties.session.namespace}resolves to a valid property chain. TheredisPropertiesbean provides access tosession.namespacethrough theEngineRedisSessionPropertiesclass, which inherits thenamespaceproperty from Spring Boot'sRedisSessionProperties. This property path is already actively used in the codebase (e.g.,DataConfigurationProperties.javalines 111–113), confirming the configuration is properly initialized and functional.Dockerfile (1)
1-1: LGTM! Base image upgrade to Eclipse Temurin.Eclipse Temurin is the official OpenJDK distribution from the Eclipse Foundation and provides better long-term support and regular security updates compared to the deprecated
openjdkimages.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java (1)
19-19: LGTM! Disables automatic index creation as per project pattern.Setting
createIndex = falseprevents Spring Data Elasticsearch from automatically creating indexes, ensuring centralized index management through ElasticsearchRunner. This change is consistent with the established pattern for ElasticCase and ElasticPetriNet.Based on learnings.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java (1)
20-20: LGTM! Consistent with centralized index management pattern.The addition of
createIndex = falsealigns with the project's approach to manage Elasticsearch indexes centrally through ElasticsearchRunner, preventing conflicts with Spring Data's automatic index creation.Based on learnings.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java (1)
16-16: LGTM! Disables automatic index creation.This change is consistent with the centralized index management pattern used across all Elasticsearch domain classes in the project.
Based on learnings.
nae-object-library/pom.xml (1)
10-10: Disregard this review comment—it is based on an incorrect premise.The review comment assumes the version downgrade occurs within this PR, but verification shows no POM files are modified in the current PR. All
pom.xmlfiles are unchanged and already set to version7.0.0-RC8.2from prior commits. The git history shows a version progression (RC9 → RC8.1 → RC8.2) in earlier commits, but the current PR contains no version changes.Likely an incorrect or invalid review comment.
nae-user-common/pom.xml (1)
9-9: Verify parent POM version downgrade is intentional.Line 9 shows a downgrade from 7.0.0-RC9 to 7.0.0-RC8.2. This is atypical for a bugfix PR and may indicate a merge conflict resolution or revert. Given that the PR objective involves adding MongoDB indexes, confirm that:
- The parent POM RC8.2 contains the necessary changes for NAE-2267 (indexing annotations, configuration, etc.)
- This downgrade is intentional and not accidental (RC9 > RC8.2)
- All dependent modules using the parent consistently target RC8.2
nae-spring-core-adapter/pom.xml (1)
10-10: Verify parent POM version downgrade is consistent and intentional.Line 10 shows the same downgrade pattern (7.0.0-RC9 → 7.0.0-RC8.2) as seen in nae-user-common/pom.xml. This coordinated downgrade across modules is atypical. Confirm that:
- The parent POM RC8.2 contains all indexing-related changes required by NAE-2267
- This is a deliberate update, not an accidental revert
- All modules are consistently aligned to RC8.2
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Task.java (1)
24-31: MarkingTaskas@Indexablealigns it with central index configurationAdding
@Indexablehere is consistent with the new resolver filter and will allow your Mongo index configurator to pick up Task’s custom@Indexedfields. No issues from the class side.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/Case.java (1)
25-56: Case now indexable with indexes onvisualIdandprocessIdentifierAnnotating
Casewith@Indexableand adding custom@IndexedonvisualIdandprocessIdentifierlooks consistent with the new index configurator and should address missing Mongo indexes for common lookup fields. Assuming queries frequently filter on these, this is a solid change; no issues spotted here.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/AbstractMongoIndexesConfigurator.java (1)
37-42: No coverage gap detected – verify passedVerification confirms all three entities using custom
@Indexedannotations (PetriNet, Case, Task) are already marked@Indexable. Additionally, indexes configured viagetIndexes()are applied independently of the filter (inaddConfiguredFields()at lines 71–72), so no coverage gap exists for those either.However, the annotation definition shows
@Indexableis not marked@Inherited:public @interface Indexable { }This means if your codebase adds class hierarchies where a parent is
@Indexablebut a child is not, the child won't inherit the annotation—confirming the inheritance risk raised in the review. Consider marking@Indexablewith@Inheritedto prevent this in the future, though no current entities exploit this scenario.nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java (2)
3-4: LGTM: Indexing annotation imports added correctly.The imports for
@Indexableand@Indexedannotations are correctly added to support the indexing functionality introduced in this PR.
29-29: Class-level indexing annotation added appropriately.The
@Indexableannotation correctly marks the PetriNet class for indexing support.application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (3)
108-116: Verify Redis session namespace fallback logic against actual key namespace usageYou now special‑case
redis.getSession().getNamespace()to rewrite only when null/blank/"spring:session", and set it to"spring:session:" + databaseName. Depending on how theRedisIndexedSessionRepositorykey namespace is constructed elsewhere, this could produce a doublespring:sessionprefix or a namespace that diverges from the intended sharedNETGRIF_ENGINE_DATA_REDIS_SESSION_NAMESPACEbehavior.Please double‑check that:
- the value stored in
EngineRedisSessionProperties.namespacematches whatSessionConfiguration(and any other consumers) expect, and- the computed prefix used in
SessionManagerServiceis still aligned with the actual Redis key pattern.
277-282: Check impact of making Mongomodenullable
MongoProperties.modeis now declared without a default (private ClusterConnectionMode mode;). Any code that previously assumed a non‑null default (e.g.,SINGLE) and passes this directly into driver settings may now seenulland either NPE or silently fall back to driver defaults.Please verify usages of
MongoProperties.getMode()and ensure they either:
- handle
nullexplicitly (e.g. conditional call to builder), or- reintroduce a safe default where required.
715-844: RedisProperties refactor to nested Sentinel + session props looks correctThe split into
RedisSentinelPropertiesandEngineRedisSessionProperties(extendingRedisSessionProperties) with dedicatedsessionandsentinelfields is clear and matches the YAML structure (redis.session.namespace, sentinel nodes, auth, etc.). Defaults for SSL, sentinel nodes, and session limiting/filtering are conservative and safe.No functional issues spotted here; just ensure documentation/examples use the new nested properties.
application-engine/src/test/resources/application-test.yaml (1)
10-11: Test queue delay override is appropriateSetting
netgrif.engine.data.elasticsearch.queue.delay: 10for tests is consistent with the new queue configuration and should help tests execute faster without impacting production defaults.application-engine/src/main/resources/application.yaml (1)
25-29: Redis session namespace nesting matches intended shared‑namespace designMoving to
redis.session.namespace: ${NETGRIF_ENGINE_DATA_REDIS_SESSION_NAMESPACE:netgrif}undernetgrif.engine.data.redisaligns with the nestedEngineRedisSessionPropertiesand the previously agreed shared session namespace across deployments.Looks good; just ensure migration docs mention the new property path and env variable.
application-engine/src/main/java/com/netgrif/application/engine/manager/service/SessionManagerService.java (1)
28-32: EnsureredisUsernameKeystill matches actual Redis principal index key patternAfter moving the namespace under
redisProperties.getSession(),redisUsernameKeyis now built as:
DEFAULT_NAMESPACE + ":" + redisProperties.getSession().getNamespace() + ":index:...".Please verify this still matches the prefix actually used by
RedisIndexedSessionRepositoryfor the principal‑name index, especially in combination with the namespace initialization inDataConfigurationProperties.redisProperties()and any session configuration class. A mismatch here would makegetAllLoggedUsers()silently return no users even when sessions exist.application-engine/src/main/java/com/netgrif/application/engine/configuration/AbstractSecurityConfiguration.java (1)
222-235: Session and filter configuration correctly switched to nested Redis session propertiesUsing
redisProperties.getSession().isEnabledLimitSession()/getMaxSession()/isEnabledFilter()is consistent with the newEngineRedisSessionPropertiesstructure and should preserve existing behavior with the new configuration layout.No issues spotted here.
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticIndexService.java (1)
448-458: UsingelasticsearchConverter.mapObject(doc)for bulk updates is a sound fixSwitching the bulk update payload to
elasticsearchTemplate.getElasticsearchConverter().mapObject(doc)for both cases and tasks should align the serialized document structure with Spring Data Elasticsearch’s mapping expectations and the new Java client, while still usingdoc.getId()anddocAsUpsert(true)as before.Please verify via tests that the resulting JSON matches the index mapping (no missing fields or unexpected structure changes).
Also applies to: 470-480
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfigurationStaticEnabled.java (1)
11-16: Redis session namespace placeholder is consistent with shared session configurationUsing
spring:session:${netgrif.engine.data.redis.session.namespace}aligns this static configuration with the shared, configurable Redis namespace used elsewhere in the engine and keeps session namespace handling centralized.application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java (1)
23-40: Constructor-based wiring of ElasticCaseService and related dependencies looks soundMoving to
@RequiredArgsConstructorwithfinalfields and passing all required collaborators (ElasticCaseRepository,ElasticsearchTemplate, the appropriateExecutor,ElasticsearchProperties,IPetriNetService,IWorkflowService,IElasticCasePrioritySearch,ApplicationEventPublisher,ElasticsearchClient) intoElasticCaseServiceis consistent and much clearer than field injection.Primary and reindexing case services differ only by executor, which matches the intent of using separate thread pools for normal vs reindex operations.
Just keep the
ElasticCaseServiceconstructor signature in sync with this configuration when evolving it, as any mismatch will only surface at compile time.Also applies to: 57-71, 79-91
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/MapField.java (1)
18-34: Translation-aware map constructor LGTMThe constructor cleanly captures keys plus I18n translations and keeps text/fulltext arrays in sync. Looks solid.
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseMappingService.java (1)
96-139: Nice reuse of I18nString utilitiesSwitching multichoice/enum mapping to I18nString plus I18nStringUtils keeps translation handling centralized and null-safe. Good refactor.
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (1)
98-122: Queue integration and shutdown wiring look goodWiring the dedicated index/delete queues and stopping them in @PreDestroy keeps the service tidy. Nice work.
|
|
||
| /** | ||
| * Configuration properties for handling queues in Elasticsearch operations. | ||
| * These properties specify the behavior of the ElasticQueueManager, | ||
| * including the maximum queue size, delay between flush operations, | ||
| * and the thread pool size for scheduled executor service tasks. | ||
| */ | ||
| @Valid | ||
| private QueueProperties queue = new QueueProperties(); | ||
|
|
There was a problem hiding this comment.
Elasticsearch queue properties look consistent; fix Javadoc type reference
The new queue configuration and QueueProperties (pool size, delay, maxBatchSize, maxQueueSize, Refresh refreshPolicy) are coherent and provide sensible defaults. However, the Javadoc for refreshPolicy still references {@link RefreshPolicy#NONE} while the field type is co.elastic.clients.elasticsearch._types.Refresh with default Refresh.False.
Recommend updating the Javadoc to reference the correct type/constant (e.g. Refresh.False) to avoid confusion.
Also applies to: 657-711
| if (hasCredentials(redisProperties.getUsername(), redisProperties.getPassword())) { | ||
| redisStandaloneConfiguration.setUsername(redisProperties.getUsername()); | ||
| redisStandaloneConfiguration.setPassword(redisProperties.getPassword()); | ||
| } | ||
| return new JedisConnectionFactory(redisStandaloneConfiguration); | ||
| JedisClientConfiguration clientConfiguration = jedisClientConfiguration(); | ||
| return new JedisConnectionFactory(redisStandaloneConfiguration, clientConfiguration); | ||
| } | ||
|
|
||
| protected JedisConnectionFactory redisSentinelConfiguration() { | ||
| RedisSentinelConfiguration sentinelConfiguration = new RedisSentinelConfiguration(); | ||
| sentinelConfiguration.setMaster(redisProperties.getSentinel().getMaster()); | ||
| List<RedisNode> nodes = redisProperties.getSentinel().getNodes().stream().map(node -> { | ||
| try { | ||
| return RedisNode.fromString(node); | ||
| } catch (Exception e) { | ||
| log.warn("Parsing redis sentinel node {} has failed. Trying to use the value as an address without port and adding default sentinel port {}", node, RedisNode.DEFAULT_SENTINEL_PORT, e); | ||
| return new RedisNode(node, RedisNode.DEFAULT_SENTINEL_PORT); | ||
| } | ||
| }).toList(); | ||
| sentinelConfiguration.setSentinels(nodes); | ||
|
|
||
| if (hasCredentials(redisProperties.getUsername(), redisProperties.getPassword())) { | ||
| sentinelConfiguration.setUsername(redisProperties.getUsername()); | ||
| sentinelConfiguration.setPassword(redisProperties.getPassword()); | ||
| } | ||
| if (hasCredentials(redisProperties.getSentinel().getUsername(), redisProperties.getSentinel().getPassword())) { | ||
| sentinelConfiguration.setSentinelUsername(redisProperties.getSentinel().getUsername()); | ||
| sentinelConfiguration.setSentinelPassword(redisProperties.getSentinel().getPassword()); | ||
| } | ||
|
|
||
| JedisClientConfiguration clientConfiguration = jedisClientConfiguration(); | ||
| return new JedisConnectionFactory(sentinelConfiguration, clientConfiguration); | ||
| } | ||
|
|
||
| protected JedisClientConfiguration jedisClientConfiguration() { | ||
| if (redisProperties.isSsl()) { | ||
| return JedisClientConfiguration.builder().useSsl().build(); | ||
| } | ||
| return JedisClientConfiguration.defaultConfiguration(); | ||
| } | ||
|
|
||
| private boolean hasCredentials(String username, String password) { | ||
| return username != null && !username.isBlank() && | ||
| password != null && !password.isBlank(); | ||
| } |
There was a problem hiding this comment.
Do not require username to send Redis passwords
hasCredentials forces both username and password to be non-blank before we call setPassword(...). Many deployments (including the default Redis user) supply only a password. After this change their password is silently dropped and every connection will fail with NOAUTH, which is a hard regression.
Set username and password independently so password-only auth keeps working (same applies to sentinel credentials).
- if (hasCredentials(redisProperties.getUsername(), redisProperties.getPassword())) {
- redisStandaloneConfiguration.setUsername(redisProperties.getUsername());
- redisStandaloneConfiguration.setPassword(redisProperties.getPassword());
- }
+ if (hasText(redisProperties.getUsername())) {
+ redisStandaloneConfiguration.setUsername(redisProperties.getUsername());
+ }
+ if (hasText(redisProperties.getPassword())) {
+ redisStandaloneConfiguration.setPassword(redisProperties.getPassword());
+ }
@@
- if (hasCredentials(redisProperties.getUsername(), redisProperties.getPassword())) {
- sentinelConfiguration.setUsername(redisProperties.getUsername());
- sentinelConfiguration.setPassword(redisProperties.getPassword());
- }
- if (hasCredentials(redisProperties.getSentinel().getUsername(), redisProperties.getSentinel().getPassword())) {
- sentinelConfiguration.setSentinelUsername(redisProperties.getSentinel().getUsername());
- sentinelConfiguration.setSentinelPassword(redisProperties.getSentinel().getPassword());
- }
+ if (hasText(redisProperties.getUsername())) {
+ sentinelConfiguration.setUsername(redisProperties.getUsername());
+ }
+ if (hasText(redisProperties.getPassword())) {
+ sentinelConfiguration.setPassword(redisProperties.getPassword());
+ }
+ if (hasText(redisProperties.getSentinel().getUsername())) {
+ sentinelConfiguration.setSentinelUsername(redisProperties.getSentinel().getUsername());
+ }
+ if (hasText(redisProperties.getSentinel().getPassword())) {
+ sentinelConfiguration.setSentinelPassword(redisProperties.getSentinel().getPassword());
+ }
@@
- private boolean hasCredentials(String username, String password) {
- return username != null && !username.isBlank() &&
- password != null && !password.isBlank();
+ private boolean hasText(String value) {
+ return value != null && !value.isBlank();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (hasCredentials(redisProperties.getUsername(), redisProperties.getPassword())) { | |
| redisStandaloneConfiguration.setUsername(redisProperties.getUsername()); | |
| redisStandaloneConfiguration.setPassword(redisProperties.getPassword()); | |
| } | |
| return new JedisConnectionFactory(redisStandaloneConfiguration); | |
| JedisClientConfiguration clientConfiguration = jedisClientConfiguration(); | |
| return new JedisConnectionFactory(redisStandaloneConfiguration, clientConfiguration); | |
| } | |
| protected JedisConnectionFactory redisSentinelConfiguration() { | |
| RedisSentinelConfiguration sentinelConfiguration = new RedisSentinelConfiguration(); | |
| sentinelConfiguration.setMaster(redisProperties.getSentinel().getMaster()); | |
| List<RedisNode> nodes = redisProperties.getSentinel().getNodes().stream().map(node -> { | |
| try { | |
| return RedisNode.fromString(node); | |
| } catch (Exception e) { | |
| log.warn("Parsing redis sentinel node {} has failed. Trying to use the value as an address without port and adding default sentinel port {}", node, RedisNode.DEFAULT_SENTINEL_PORT, e); | |
| return new RedisNode(node, RedisNode.DEFAULT_SENTINEL_PORT); | |
| } | |
| }).toList(); | |
| sentinelConfiguration.setSentinels(nodes); | |
| if (hasCredentials(redisProperties.getUsername(), redisProperties.getPassword())) { | |
| sentinelConfiguration.setUsername(redisProperties.getUsername()); | |
| sentinelConfiguration.setPassword(redisProperties.getPassword()); | |
| } | |
| if (hasCredentials(redisProperties.getSentinel().getUsername(), redisProperties.getSentinel().getPassword())) { | |
| sentinelConfiguration.setSentinelUsername(redisProperties.getSentinel().getUsername()); | |
| sentinelConfiguration.setSentinelPassword(redisProperties.getSentinel().getPassword()); | |
| } | |
| JedisClientConfiguration clientConfiguration = jedisClientConfiguration(); | |
| return new JedisConnectionFactory(sentinelConfiguration, clientConfiguration); | |
| } | |
| protected JedisClientConfiguration jedisClientConfiguration() { | |
| if (redisProperties.isSsl()) { | |
| return JedisClientConfiguration.builder().useSsl().build(); | |
| } | |
| return JedisClientConfiguration.defaultConfiguration(); | |
| } | |
| private boolean hasCredentials(String username, String password) { | |
| return username != null && !username.isBlank() && | |
| password != null && !password.isBlank(); | |
| } | |
| if (hasText(redisProperties.getUsername())) { | |
| redisStandaloneConfiguration.setUsername(redisProperties.getUsername()); | |
| } | |
| if (hasText(redisProperties.getPassword())) { | |
| redisStandaloneConfiguration.setPassword(redisProperties.getPassword()); | |
| } | |
| JedisClientConfiguration clientConfiguration = jedisClientConfiguration(); | |
| return new JedisConnectionFactory(redisStandaloneConfiguration, clientConfiguration); | |
| } | |
| protected JedisConnectionFactory redisSentinelConfiguration() { | |
| RedisSentinelConfiguration sentinelConfiguration = new RedisSentinelConfiguration(); | |
| sentinelConfiguration.setMaster(redisProperties.getSentinel().getMaster()); | |
| List<RedisNode> nodes = redisProperties.getSentinel().getNodes().stream().map(node -> { | |
| try { | |
| return RedisNode.fromString(node); | |
| } catch (Exception e) { | |
| log.warn("Parsing redis sentinel node {} has failed. Trying to use the value as an address without port and adding default sentinel port {}", node, RedisNode.DEFAULT_SENTINEL_PORT, e); | |
| return new RedisNode(node, RedisNode.DEFAULT_SENTINEL_PORT); | |
| } | |
| }).toList(); | |
| sentinelConfiguration.setSentinels(nodes); | |
| if (hasText(redisProperties.getUsername())) { | |
| sentinelConfiguration.setUsername(redisProperties.getUsername()); | |
| } | |
| if (hasText(redisProperties.getPassword())) { | |
| sentinelConfiguration.setPassword(redisProperties.getPassword()); | |
| } | |
| if (hasText(redisProperties.getSentinel().getUsername())) { | |
| sentinelConfiguration.setSentinelUsername(redisProperties.getSentinel().getUsername()); | |
| } | |
| if (hasText(redisProperties.getSentinel().getPassword())) { | |
| sentinelConfiguration.setSentinelPassword(redisProperties.getSentinel().getPassword()); | |
| } | |
| JedisClientConfiguration clientConfiguration = jedisClientConfiguration(); | |
| return new JedisConnectionFactory(sentinelConfiguration, clientConfiguration); | |
| } | |
| protected JedisClientConfiguration jedisClientConfiguration() { | |
| if (redisProperties.isSsl()) { | |
| return JedisClientConfiguration.builder().useSsl().build(); | |
| } | |
| return JedisClientConfiguration.defaultConfiguration(); | |
| } | |
| private boolean hasText(String value) { | |
| return value != null && !value.isBlank(); | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/configuration/SessionConfiguration.java
around lines 48 to 92, the current hasCredentials method requires both username
and password to be present which drops password-only Redis auth; modify the
logic so username and password are checked and set independently: if password is
non-null/non-blank call setPassword(...), if username is non-null/non-blank call
setUsername(...); apply the same independent checks for
sentinelUsername/sentinelPassword (use separate conditions or replace
hasCredentials with two small helper checks) so password-only deployments
continue to authenticate correctly.
| @Data | ||
| @Builder | ||
| @AllArgsConstructor | ||
| public class BulkOperationWrapper { | ||
|
|
||
| private BulkOperation operation; | ||
|
|
||
| private EventObject publishableEvent; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the wrapper immutable for safer use in queues/events
If BulkOperationWrapper instances are passed through queues and across threads, mutability via Lombok’s generated setters can be a source of subtle bugs. Consider making it immutable:
-@Data
-@Builder
-@AllArgsConstructor
-public class BulkOperationWrapper {
-
- private BulkOperation operation;
-
- private EventObject publishableEvent;
-}
+@lombok.Value
+@Builder
+@AllArgsConstructor
+public class BulkOperationWrapper {
+
+ BulkOperation operation;
+
+ EventObject publishableEvent;
+}This keeps the builder while removing setters and enforcing immutability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Data | |
| @Builder | |
| @AllArgsConstructor | |
| public class BulkOperationWrapper { | |
| private BulkOperation operation; | |
| private EventObject publishableEvent; | |
| } | |
| @lombok.Value | |
| @Builder | |
| @AllArgsConstructor | |
| public class BulkOperationWrapper { | |
| BulkOperation operation; | |
| EventObject publishableEvent; | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/elastic/domain/BulkOperationWrapper.java
around lines 11 to 19, the class is mutable due to Lombok’s @Data (which
generates setters); make it immutable for safe usage across threads/queues by
replacing @Data and @AllArgsConstructor with Lombok’s @Value (or annotate fields
final and class with @Getter + no setters) and keep @Builder; mark the fields as
private final (remove any setter generation), remove the all-args constructor
visibility if needed, and rebuild so instances are immutable while still
supporting builder construction.
| log.debug("Index started with batch size: {} and id: {}", batch.size(), uuid); | ||
| elasticsearchClient.bulk(new BulkRequest.Builder().operations(batch.stream().map(BulkOperationWrapper::getOperation).toList()).refresh(queueProperties.getRefreshPolicy()).build()); | ||
| log.debug("Index finished with batch size: {} and id: {}", batch.size(), uuid); | ||
| checkQueue(); | ||
| } catch (Exception e) { | ||
| log.error("Bulk operation failed for batch id: {} with {} operations. " + | ||
| "Operations will be retried via scheduled indexing or manual reindex.", | ||
| uuid, batch.size(), e); | ||
| return; | ||
| } | ||
| try { | ||
| publishEventsOfBatch(batch); | ||
| } catch (Exception e) { | ||
| log.error("Event publishing failed for batch id: {}", uuid, e); | ||
| } |
There was a problem hiding this comment.
Handle Elasticsearch bulk responses that report item failures
elasticsearchClient.bulk(...) returns a BulkResponse that can contain per-item failures without throwing. Right now we treat every call as success, drop the batch from the queue, and still publish events. If Elasticsearch returns errors=true, the documents never get indexed, yet we falsely signal success and lose the operations permanently. That’s a correctness regression.
Guard on BulkResponse.errors() before calling checkQueue() / publishEventsOfBatch, log the failure details, and bail so the fallback reindex paths can recover.
-import co.elastic.clients.elasticsearch.core.BulkRequest;
+import co.elastic.clients.elasticsearch.core.BulkRequest;
+import co.elastic.clients.elasticsearch.core.BulkResponse;
@@
- log.debug("Index started with batch size: {} and id: {}", batch.size(), uuid);
- elasticsearchClient.bulk(new BulkRequest.Builder().operations(batch.stream().map(BulkOperationWrapper::getOperation).toList()).refresh(queueProperties.getRefreshPolicy()).build());
- log.debug("Index finished with batch size: {} and id: {}", batch.size(), uuid);
+ log.debug("Index started with batch size: {} and id: {}", batch.size(), uuid);
+ BulkResponse response = elasticsearchClient.bulk(new BulkRequest.Builder()
+ .operations(batch.stream().map(BulkOperationWrapper::getOperation).toList())
+ .refresh(queueProperties.getRefreshPolicy())
+ .build());
+ if (response.errors()) {
+ log.error("Bulk operation finished with errors for batch id: {} ({} operations). Failing items will be handled by scheduled reindex or manual reindex.", uuid, batch.size());
+ return;
+ }
+ log.debug("Index finished with batch size: {} and id: {}", batch.size(), uuid);
checkQueue();| String objectId = ((Document)document.get("_id")).get("shortProcessId") + "-" + ((Document)document.get("_id")).get("objectId").toString(); | ||
| if (objectId != null) { | ||
| service.remove(objectId); | ||
| return; | ||
| } | ||
|
|
||
| objectId = document.getObjectId("petriNetObjectId").toString(); | ||
| if (objectId != null) { | ||
| service.removeByPetriNetId(objectId); | ||
| return; | ||
| } | ||
|
|
||
| throw new IllegalStateException("Case has been deleted neither by ID nor by process ID!"); | ||
| service.remove(objectId); |
There was a problem hiding this comment.
🧩 Analysis chain
Add defensive null checks to prevent NPE and ClassCastException.
The nested field access on line 60 has multiple failure points without defensive checks:
document.get("_id")may return null or a non-Document typeshortProcessIdorobjectIdfields within the_idDocument may be null- Calling
.toString()on a nullobjectIdwill throw NPE
These runtime exceptions would prevent Elasticsearch cleanup and leave orphaned documents in the index.
Apply this diff to add defensive checks:
@Override
public void onAfterDelete(AfterDeleteEvent<Case> event) {
Document document = event.getDocument();
if (document == null) {
log.warn("Trying to delete null document!");
return;
}
- String objectId = ((Document)document.get("_id")).get("shortProcessId") + "-" + ((Document)document.get("_id")).get("objectId").toString();
- service.remove(objectId);
+ Object idObject = document.get("_id");
+ if (!(idObject instanceof Document)) {
+ log.error("Document _id is not a Document, cannot delete from Elasticsearch: {}", idObject);
+ return;
+ }
+
+ Document idDoc = (Document) idObject;
+ Object shortProcessId = idDoc.get("shortProcessId");
+ Object objectId = idDoc.get("objectId");
+
+ if (shortProcessId == null || objectId == null) {
+ log.error("Missing shortProcessId or objectId in document _id, cannot delete from Elasticsearch. shortProcessId: {}, objectId: {}", shortProcessId, objectId);
+ return;
+ }
+
+ String compositeId = shortProcessId + "-" + objectId.toString();
+ try {
+ service.remove(compositeId);
+ } catch (Exception e) {
+ log.error("Failed to remove case from Elasticsearch with id: {}", compositeId, e);
+ }
}Additionally, verify that IElasticCaseService.remove() correctly handles the "shortProcessId-objectId" format, and ensure this matches the format used in other event handlers (e.g., TaskEventHandler if it exists).
🏁 Script executed:
#!/bin/bash
# Description: Verify the objectId format is consistently used across event handlers and service methods.
# Search for other event handlers that construct similar composite IDs
echo "=== Searching for similar event handlers ==="
rg -n -A 5 -B 2 'onAfterDelete.*AfterDeleteEvent' --type java
echo -e "\n=== Searching for IElasticCaseService.remove usage ==="
ast-grep --pattern 'service.remove($_)'
echo -e "\n=== Searching for shortProcessId concatenation patterns ==="
rg -n 'shortProcessId.*-.*objectId' --type javaLength of output: 4437
I need to examine TaskEventHandler and verify the full context of both handlers, as the output shows they use identical patterns.
Now I'll examine the full context of both event handlers and search for the service method definition:
🏁 Script executed:
#!/bin/bash
# Get full context of both event handlers and verify _id structure
echo "=== CaseEventHandler.java (full onAfterDelete method) ==="
cat application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseEventHandler.java
echo -e "\n=== TaskEventHandler.java (full onAfterDelete method) ==="
cat application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskEventHandler.java
echo -e "\n=== Searching for IElasticCaseService.remove() definition ==="
rg -n 'void remove\|remove\(' --type java -B 2 -A 8 | grep -A 8 'interface IElastic\|class.*IElastic'
echo -e "\n=== Searching for Case domain class to understand _id structure ==="
find application-engine -name 'Case.java' -type f | head -5
echo -e "\n=== Searching for Task domain class to understand _id structure ==="
find application-engine -name 'Task.java' -type f | head -5Length of output: 4972
Add defensive null checks and fallback logic to match TaskEventHandler pattern.
The code on lines 60-61 lacks safeguards that the parallel TaskEventHandler implements. TaskEventHandler has:
- A null check:
if (document.get("_id") != null)before nested field access - Fallback removal logic via
processId - Error handling with
IllegalStateExceptionif both approaches fail
CaseEventHandler is missing these, creating NPE and ClassCastException risks:
document.get("_id")may be null, returning null when cast without check- Nested fields
shortProcessIdandobjectIdwithin the_idDocument may be null - No fallback if composite ID cannot be constructed
These vulnerabilities would prevent Elasticsearch cleanup and leave orphaned documents.
Apply this diff to align with TaskEventHandler's defensive pattern:
@Override
public void onAfterDelete(AfterDeleteEvent<Case> event) {
Document document = event.getDocument();
if (document == null) {
log.warn("Trying to delete null document!");
return;
}
- String objectId = ((Document)document.get("_id")).get("shortProcessId") + "-" + ((Document)document.get("_id")).get("objectId").toString();
- service.remove(objectId);
+ if (document.get("_id") != null) {
+ String objectId = ((Document)document.get("_id")).get("shortProcessId") + "-" + ((Document)document.get("_id")).get("objectId").toString();
+ service.remove(objectId);
+ return;
+ }
+
+ String processId = document.getString("processId");
+ if (processId != null) {
+ service.removeByPetriNetId(processId);
+ return;
+ }
+
+ throw new IllegalStateException("Case has been deleted neither by ID nor by process ID!");
}Committable suggestion skipped: line range outside the PR's diff.
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({ElementType.TYPE}) | ||
| public @interface Indexable { | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making @Indexable @Inherited to support entity hierarchies
Right now @Indexable is not @Inherited, so subclasses won’t be treated as indexable unless they also declare the annotation. If your Mongo root entities are often concrete subclasses of abstract bases like Case, Task, or PetriNet, adding @Inherited would reduce the risk of missing indexes when new subclasses are introduced.
Example:
@Inherited
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface Indexable {
}If you intentionally want only explicitly annotated concrete types to be indexable, the current setup is fine.
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/annotations/Indexable.java
around lines 3 to 11, the annotation is not marked @Inherited so subclasses
won't inherit it; add the java.lang.annotation.Inherited import and annotate the
interface with @Inherited (keeping existing @Retention(RetentionPolicy.RUNTIME)
and @Target(ElementType.TYPE)) so concrete subclasses of annotated base types
are treated as indexable.
| public class I18nStringUtils { | ||
| public static List<String> collectTranslations(I18nString i18nString) { | ||
| List<String> translations = new ArrayList<>(); | ||
| if (i18nString == null) { | ||
| return translations; | ||
| } | ||
| translations.add(i18nString.getDefaultValue()); | ||
| translations.addAll(i18nString.getTranslations().values()); | ||
| return translations; | ||
| } |
There was a problem hiding this comment.
Guard against null translations/defaults to avoid NPEs and null entries
Current implementation assumes i18nString.getTranslations() and getDefaultValue() are non‑null. If either can be null, this will either throw an NPE or add nulls into the result list.
Consider tightening defensively:
public static List<String> collectTranslations(I18nString i18nString) {
List<String> translations = new ArrayList<>();
if (i18nString == null) {
return translations;
}
- translations.add(i18nString.getDefaultValue());
- translations.addAll(i18nString.getTranslations().values());
+ if (i18nString.getDefaultValue() != null) {
+ translations.add(i18nString.getDefaultValue());
+ }
+ if (i18nString.getTranslations() != null) {
+ translations.addAll(i18nString.getTranslations().values());
+ }
return translations;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class I18nStringUtils { | |
| public static List<String> collectTranslations(I18nString i18nString) { | |
| List<String> translations = new ArrayList<>(); | |
| if (i18nString == null) { | |
| return translations; | |
| } | |
| translations.add(i18nString.getDefaultValue()); | |
| translations.addAll(i18nString.getTranslations().values()); | |
| return translations; | |
| } | |
| public class I18nStringUtils { | |
| public static List<String> collectTranslations(I18nString i18nString) { | |
| List<String> translations = new ArrayList<>(); | |
| if (i18nString == null) { | |
| return translations; | |
| } | |
| if (i18nString.getDefaultValue() != null) { | |
| translations.add(i18nString.getDefaultValue()); | |
| } | |
| if (i18nString.getTranslations() != null) { | |
| translations.addAll(i18nString.getTranslations().values()); | |
| } | |
| return translations; | |
| } | |
| } |
|
|
||
| @Getter | ||
| @Setter | ||
| @Indexed |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that initials is the optimal field to index.
The @Indexed annotation on the initials field addresses the missing indexes issue. However, the identifier field on line 36 has a comment suggesting it should be part of a compound index with version for uniqueness enforcement. Consider whether identifier should also be indexed independently or as part of a compound index, depending on query patterns.
Run the following script to analyze query patterns for both initials and identifier fields:
🏁 Script executed:
#!/bin/bash
# Description: Search for query patterns using initials and identifier fields to determine indexing priorities
echo "=== Searching for queries using 'initials' field ==="
rg -n --type=java -C3 'findBy.*Initials|initials.*equals|criteria.*initials|query.*initials' -i
echo -e "\n=== Searching for queries using 'identifier' field ==="
rg -n --type=java -C3 'findBy.*Identifier|identifier.*equals|criteria.*identifier|query.*identifier' -i
echo -e "\n=== Checking PetriNet repository/query usage ==="
ast-grep --pattern 'class $_ implements $$$Repository<PetriNet$$$> {
$$$
}'Length of output: 38196
Now I need to inspect the PetriNet.java file to see the actual index definitions and the comment mentioned in the review:<!-- [scratchpad_continue]
[key_finding_from_web_search]
- MongoDB compound indexes support queries on the index prefix: a compound index on {identifier: 1, version: 1} can efficiently support:
- Queries on identifier alone (prefix match)
- Queries on identifier AND version together
- But NOT queries on version alone (violates index prefix requirement)
- If query patterns are:
findByIdentifier()- single field queryfindByIdentifierAndVersion()- compound query
Then a compound index on (identifier, version) covers both efficiently
[critical_missing_info]
- Need to see PetriNet.java file to check:
- Current index definitions on
identifierfield - Whether a compound index with
versionalready exists - The exact comment on line 36 mentioning the index strategy
- Whether
@Indexedannotation is already onidentifier
- Current index definitions on
[next_action]
- Search PetriNet.java for index definitions and line 36 comment
[scratchpad_continue_end] -->
Now let me inspect the PetriNet.java file to see the current index definitions:
[/run_scripts]
Based on the query pattern analysis from the script output, here are my findings:
Query patterns for initials:
- Single regex query:
Criteria.where("initials").regex()(line 484 in PetriNetService.java) - Test assertion on retrieved object
Query patterns for identifier:
- Direct repository lookups:
findByIdentifier(),findByIdentifierAndVersion()(Repository methods) - Regex queries:
Criteria.where("identifier").regex()(line 478) - Aggregation/grouping:
Criteria.where("identifier").in()(line 305) - Multiple service calls using the identifier queries (lines 276, 289)
- Heavy usage across search criteria (line 477)
The evidence shows:
initialsfield: Used only in regex-based search criteria →@Indexedannotation is appropriate for this single-field query patternidentifierfield: Used extensively in both single-field lookups (findByIdentifier()) AND compound queries (findByIdentifierAndVersion()). The comment on line 36 suggests a compound index withversionfor uniqueness enforcement, which aligns with MongoDB's compound index prefix capabilities—a compound index on (identifier, version) efficiently supports both single-field lookups on identifier and compound lookups.
Verify that initials is the optimal field to index.
The @Indexed annotation on initials is appropriate for the current query patterns, which primarily use single-field regex-based searches. However, query analysis reveals the identifier field is used far more extensively, including both standalone lookups (findByIdentifier()) and compound lookups with version (findByIdentifierAndVersion()). The comment on line 36 suggests identifier should be part of a compound index with version for uniqueness enforcement, which is architecturally sound: a compound index can support queries on all indexed fields including the first field.
Consider adding a compound index on (identifier, version) to support both query patterns efficiently, rather than indexing identifier alone. This aligns with the design intent noted in the comment and would improve performance for the frequently-used findByIdentifierAndVersion() lookups while also supporting findByIdentifier() queries via the index prefix.
🏁 Script executed:
#!/bin/bash
# Extract the PetriNet class definition with focus on indexes and line 36
cat nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java | head -100Length of output: 2927
Add compound index on identifier and version fields to address the stated uniqueness requirement and optimize query performance.
The PR adds @Indexed to initials, but query pattern analysis reveals a critical gap: the identifier field is used far more extensively (6+ distinct query patterns including findByIdentifierAndVersion() and findByIdentifier()), yet has no index. The comment on line 36 explicitly states that "combination of identifier and version must be unique" and questions whether @CompoundIndex should be used—indicating this was the original intent.
The current implementation indexes only initials while leaving the heavier-queried identifier field unindexed. A compound index on (identifier, version) would:
- Enforce the stated uniqueness constraint
- Support both single-field
findByIdentifier()lookups (via index prefix) and compoundfindByIdentifierAndVersion()queries - Dramatically improve query performance for the most frequently-used lookups
Add the missing compound index definition to the class level (using Spring Data's @CompoundIndex or MongoDB's equivalent) before or alongside the @Indexed annotation on initials.
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/petrinet/domain/PetriNet.java
around line 63, add a class-level compound index for the identifier and version
fields to enforce their uniqueness and improve query performance; specifically,
annotate the class with Spring Data's @CompoundIndex (or @CompoundIndexes)
declaring fields = "{'identifier': 1, 'version': 1}" and unique = true, leaving
the existing @Indexed on initials intact so both the compound
(identifier+version) and the initials index coexist.
Description
Fixes NAE-2267
Dependencies
No new dependencies were introduced
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements