Skip to content

[NAE-2442] Fix dataset update from database and action handling#444

Open
renczesstefan wants to merge 2 commits into
release/7.0.0-rev10from
NAE-2442
Open

[NAE-2442] Fix dataset update from database and action handling#444
renczesstefan wants to merge 2 commits into
release/7.0.0-rev10from
NAE-2442

Conversation

@renczesstefan
Copy link
Copy Markdown
Member

@renczesstefan renczesstefan commented May 27, 2026

Description

Fixes NAE-2442

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, and with unit tests.

Test Configuration

Name Tested on
OS macOS Tahoe 26.3
Runtime Java 21
Dependency Manager Maven 3.9.9n
Framework version Spring Boot 3.4.4
Run parameters
Other configuration

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes have been checked, personally or remotely, with @machacjozef
  • I have commented my code, particularly in hard-to-understand areas
  • I have resolved all conflicts with the target branch of the PR
  • I have updated and synced my code with the target branch
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes:
    • Lint test
    • Unit tests
    • Integration tests
  • I have checked my contribution with code analysis tools:
  • I have made corresponding changes to the documentation:
    • Developer documentation
    • User Guides
    • Migration Guides

Summary by CodeRabbit

  • Bug Fixes

    • Workflow now refreshes case state from storage before executing transition and follow-up actions, reducing stale-data issues during concurrent operations.
  • New Features

    • Action payloads now accept non-string values, allowing richer data types when setting task data.

Review Change Stack

Introduced `updateCaseFromDb` in `IWorkflowService` and its implementation in `WorkflowService` to synchronize case data with the database. Updated `EventService` to invoke this method during action execution.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f86fec9d-1c5e-448d-a1a1-5208019631af

📥 Commits

Reviewing files that changed from the base of the PR and between e86a370 and ce8003c.

📒 Files selected for processing (3)
  • application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java
  • application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
  • nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/actions/ActionApi.java

Walkthrough

Adds updateCaseFromDb to refresh an in-memory Case from the persisted Case and integrates that refresh into EventService run paths; also widens ActionApi.setData and ActionApiImpl.setData parameter types to accept non-string values (Map<String, Object>).

Changes

Case state refresh before action execution

Layer / File(s) Summary
Case refresh contract and implementation
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.java, application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
IWorkflowService declares updateCaseFromDb(Case useCase). WorkflowService implements it to load the persisted Case and merge DataSet entries that are newer in the DB into the provided in-memory Case without persisting.
EventService integration
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/EventService.java
Both runActions overloads now call workflowService.updateCaseFromDb(useCase) when useCase is non-null before executing actions.

Action API setData type widening

Layer / File(s) Summary
ActionApi signature
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/actions/ActionApi.java
setData inner map value type changed from String to Object in the public adapter interface.
ActionApiImpl signature
application-engine/src/main/java/com/netgrif/application/engine/actions/ActionApiImpl.java
setData implementation signature updated to accept Map<String, Map<String, Object>> and continues to serialize the payload before delegating to dataService.setData.

Sequence Diagram

sequenceDiagram
  participant EventService
  participant WorkflowService
  participant Repository
  EventService->>WorkflowService: updateCaseFromDb(useCase)
  WorkflowService->>Repository: find case by ID
  Repository-->>WorkflowService: persisted case
  WorkflowService->>WorkflowService: merge newer dataset fields
  WorkflowService-->>EventService: refresh complete
  EventService->>EventService: execute actions
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

bugfix, api-change, Medium

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main changes: dataset update from database and action handling improvements, which aligns with the core modifications in EventService, WorkflowService, and ActionApi.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Retoocs
Retoocs previously approved these changes May 27, 2026
@coderabbitai coderabbitai Bot added bugfix A change that fixes a bug breaking change Fix or feature that would cause existing functionality doesn't work as expected Medium labels May 27, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/EventService.java`:
- Around line 54-56: The refresh (updateCaseFromDb) is being called after
actionsRunner.run(...), causing actions to run against stale state; move the
call to workflowService.updateCaseFromDb(useCase) to immediately before each
actionsRunner.run(...) invocation within EventService so the case is reloaded
from the DB prior to executing actions (apply the same change at the other
occurrence referenced around the second actionsRunner.run(...) block as well).

In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java`:
- Around line 255-263: updateCaseFromDb currently assigns persisted DataField
objects from findOneNoNet directly into the runtime Case, which can mix
encrypted persisted fields into in-memory state; instead, when a persisted
DataField (from actual.getDataSet()) is newer than the corresponding runtime
field, copy only the safe primitive/value and metadata into the existing runtime
DataField (do not replace the object reference). In practice: in
updateCaseFromDb, for each id where
dataField.isNewerThen(useCase.getDataField(id)), obtain the target DataField =
useCase.getDataField(id) (or create a new non-persisted instance) and call its
setters to copy value, plainValue/decodedValue, timestamp, and other
non-persistence-specific metadata from dataField, rather than doing
useCase.getDataSet().put(id, dataField); this preserves in-memory
decryption/representation and avoids injecting persisted/encrypted objects
returned by findOneNoNet.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 72387161-fb2d-4021-8388-5c4ed913dee0

📥 Commits

Reviewing files that changed from the base of the PR and between 837a70b and e86a370.

📒 Files selected for processing (3)
  • application-engine/src/main/java/com/netgrif/application/engine/workflow/service/EventService.java
  • application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
  • application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.java

machacjozef
machacjozef previously approved these changes May 27, 2026
Refactored `setData` method in `ActionApi` to use `Map<String, Map<String, Object>>` for flexible data handling. Adjusted `WorkflowService` to use the `findOne` method for better database case update synchronization.
@renczesstefan renczesstefan dismissed stale reviews from machacjozef and Retoocs via ce8003c May 27, 2026 13:29
@renczesstefan
Copy link
Copy Markdown
Member Author

@coderabbitai pls review once more and approve if it is OK

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff
gh pr diff 444 --repo netgrif/application-engine

Length of output: 5982


🏁 Script executed:

#!/bin/bash
# Look at the changed files
fd -e java -t f | xargs grep -l "updateCaseFromDb\|setData" 2>/dev/null | head -20

Length of output: 2289


🏁 Script executed:

#!/bin/bash
# Check isNewerThen method on DataField
rg -n "isNewerThen" --type java -C 5

# Check findOne implementation
rg -n "findOne" application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java -A 5

# Check getDataField for null safety
rg -n "getDataField" --type java -C 3 | head -60

# Check callers of setData with old/new signature
rg -n "setData" --type java -C 2 | grep -v "\.class\|//\|\.xml" | head -60

Length of output: 24185



@renczesstefan I've reviewed all the changes thoroughly. Here's my assessment:


Review Summary

1. WorkflowService.updateCaseFromDb (new method)

  • Uses findOne which throws IllegalArgumentException if the case is not found — so actual is never null. ✅
  • Version-based conflict resolution via isNewerThen is correct: fields modified by the running action will have a higher in-memory version than the persisted DB version, so they won't be overwritten. ✅
  • Minor: useCase.getDataField(id) could theoretically return null for a field present in the DB but absent in the in-memory case, which would cause an NPE inside isNewerThen. However, this is the same pre-existing pattern already in DataService.updateDataset (lines 869–874) — this PR doesn't introduce new risk.
  • Minor: updateCaseFromDb duplicates DataService.updateDataset logic. Worth a follow-up refactor, but not a blocker.

2. EventService.runActions / runEventActions

  • The placement of updateCaseFromDb (after actionsRunner.run(...), before processing SetDataEventOutcome) is correct. ✅
  • null guard on useCase is present. ✅
  • Each action iteration now triggers a DB read — acceptable given the correctness requirement.

3. setData signature widening (Map<String, Map<String, String>>Map<String, Map<String, Object>>)

  • Breaking change from a Java generic perspective, but since NAE actions are Groovy-based (dynamic typing), existing action scripts are unaffected. ✅
  • Both ActionApi interface and ActionApiImpl are consistently updated. ✅

The changes correctly fix the data-loss issue caused by stale in-memory state during multi-action execution. No blocking issues found.

[approve]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Fix or feature that would cause existing functionality doesn't work as expected bugfix A change that fixes a bug Medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants