SED-4617 implement-detailed-inline-node-details-mode-for-step-list by david-stephan · Pull Request #644 · exense/step

david-stephan · 2026-05-13T13:53:18Z

No description provided.

…tails-mode-for-step-list

gemini-code-assist

Code Review

This pull request replaces the string-based path field in ReportNode with a structured ancestorIds set to optimize hierarchical data access and introduces a migration task to update existing records. It also implements a server-side enrichment mechanism for report nodes and adds a new endpoint to retrieve nodes with contributing errors. Feedback highlights significant performance concerns, including an N+1 query issue in the ReportNodeTableEnricher and the need for bulk operations in the migration task. Other critical suggestions include ensuring deterministic field ordering in compound indexes by avoiding Set.of, addressing an inefficient index scan when querying by ancestor IDs, and improving maintainability by replacing hardcoded field names with constants.

gemini-code-assist · 2026-05-13T13:56:08Z

+    private List<ReportNode> collectContributingErrors(ReportNode root) {
+        try (Stream<ReportNode> reportNodesWithContributingErrors = reportNodeAccessor.getReportNodesWithContributingErrorsByAncestor(root.getId().toHexString(), 0, 10)) {
+            return reportNodesWithContributingErrors.collect(Collectors.toList());
+        }
+    }


This implementation introduces an N+1 query problem. For every failed non-leaf node in a table result set, a separate database query is executed to fetch contributing errors. If a page contains many such nodes, this will significantly degrade performance. Additionally, the limit of 10 is hardcoded; per the general rules, this should be configurable via the configuration/properties.

References

Instead of hardcoding values like limits or counts, make them configurable by loading them from a properties file (e.g., step.properties) and provide a default value.

The n+1 problematic is known. With the current access layer compatible with different DB backend including NO SQL it is not possible to perform join queries and this remains the only implementation. This why this it only done for specific nodes and only when explicitly requested with the table parameter flag enrichWithContributingErrors. A new field enrichWithContributingErrorsLimit with a default value of 10 has been added to ReportNodesTableParameters allowing the configuration of the limit per request.

jeromecomte · 2026-05-18T07:48:44Z

+    private static boolean shouldEnrich(ReportNode reportNode, ReportNodesTableParameters tableParameters) {
+        return tableParameters.isEnrichWithContributingErrors()
+            && (!reportNode.isLeafReportNode())
+            && ((ReportNodeStatus.FAILED.equals(reportNode.getStatus())) || (ReportNodeStatus.TECHNICAL_ERROR.equals(reportNode.getStatus())));


Just realized something while reading this: in the legacy view, if I'm not wrong, we were also displaying assertions of keyword calls for passed calls. In this case we wouldn't show them anymore.

Regardless of the legacy view, I'm asking me if showing the executed assertions in passed calls couldn't be required in some cases for transparency / auditing reasons

I wasn't aware of this feature, but indeed if you expand the callKeyword report in the legacy step table the children are retrieved and the passed assertion are displayed too; this was OK in term of performance as only done on demands when expending one report node.
Whether this feature is a requirement, I cannot tell, but I would definitely not use the table enricher for that as it would perform additional queries for each rows (or at least it should be optional too). Should that be a requirement, I would suggest to create a dedicated request for change and see how to best make this information available.

Agree. Let's discuss this as a follow-up task

jeromecomte · 2026-05-18T08:00:33Z

    public EchoReportNode createReportNode_(ReportNode parentNode, Echo testArtefact) {
-        return new EchoReportNode();
+        EchoReportNode reportNode = new EchoReportNode();
+        reportNode.setLeafReportNode(true);


As this is static, it would be better to store this in the metadata of the Artefact annotation and access it via reportNode.resolvedArtefact

I did it here because the usage is more reporting related for now, I also thought that's what we discussed. We decided to let the handler set the value because the current model is shaky as any artefact can technically have children even if they are completely ignored by the execution of the handler.
I'm fine moving it to AbstractArtefact and its implemeting classes but in that case I would just have a default value in the abstract class and overrides with a JsonIgnore has it doesn't have to be persisted to DB.
What do you think ? and what did you mean with Artefact "annotation".

I remember the discussion but though about it again.
If we later want to define some artifacts as leaf-only, the best approach would be to store this information in the step.core.artefacts.Artefact annotation (not the AbstractArtefact itself). This way we could also use the information in the plan editor / validation to enforce it.

I completely forgot that we have this annotation, I'll implement as suggested

jeromecomte · 2026-05-18T08:02:46Z

        if (reportNode.getError() != null) {
-            nodeBucket.put(ERROR_CODE, reportNode.getError().getCode());
-            nodeBucket.put(ERROR_MESSAGE, reportNode.getError().getMsg());
+            nodeBucket.put(ERROR_CODE, reportNode.getError().getCode() != null ? reportNode.getError().getCode() : 0);


If 0 should be the default error code, shouldn't we define this default in the Error class?

@jeromecomte it definitely should have been, but is currently hardcoded all ove Step code base. I can do it but it will require a branch on the Step KW API then.

fine for me too if we want to do it later

…fact annotation

…tails-mode-for-step-list

david-stephan added 5 commits May 5, 2026 17:21

SED-4617 Implement detailed inline node details mode for Step list

8ddca51

SED-4617 user wrapper to add the assertionReportNodesOnError field

4e049db

SED-4617 using find lazy for contributing errors

67ce064

Merge branch 'master' into SED-4617-implement-detailed-inline-node-de…

57fe1c1

…tails-mode-for-step-list

SED-4617 migrating from string path to ancestor IDs

a336c20

david-stephan requested a review from jeromecomte May 13, 2026 13:53

gemini-code-assist Bot reviewed May 13, 2026

View reviewed changes

david-stephan added 2 commits May 13, 2026 16:04

SED-4617 reduced default limit of the contributing error service

556d86e

SED-4617 PR feedbacks, index optims after tests

7a0cb2f

jeromecomte reviewed May 18, 2026

View reviewed changes

david-stephan and others added 4 commits May 18, 2026 11:17

SED-4617 BE OS PR feedback part 1

debb98a

SED-4617 Refactoring default error code, and moving leaf info to arte…

ffe2f93

…fact annotation

SED-4617 reusing the existing block flag of artefact annotation

53c21c9

Merge branch 'master' into SED-4617-implement-detailed-inline-node-de…

44828c0

…tails-mode-for-step-list

jeromecomte approved these changes May 20, 2026

View reviewed changes

Conversation

david-stephan commented May 13, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants