Skip to content

Add parent action name to ProcessorGenerationContext for system gener…#22195

Open
shatejas wants to merge 2 commits into
opensearch-project:mainfrom
shatejas:parent-action-should-generate
Open

Add parent action name to ProcessorGenerationContext for system gener…#22195
shatejas wants to merge 2 commits into
opensearch-project:mainfrom
shatejas:parent-action-should-generate

Conversation

@shatejas

Copy link
Copy Markdown
Contributor

…ated processors

This exposes the parent transport action (e.g., _msearch, _async_search) to SystemGeneratedFactory.shouldGenerate() so processors can conditionally generate based on the calling action.

Related Issues

Resolves #22165

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…ated processors

This exposes the parent transport action (e.g., _msearch, _async_search) to SystemGeneratedFactory.shouldGenerate() so processors can conditionally generate based on the calling action.

Signed-off-by: Tejas Shah <shatejas@amazon.com>
@github-actions github-actions Bot added enhancement Enhancement or improvement to existing feature or request Search Search query, autocomplete ...etc labels Jun 16, 2026
@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 684e48c)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

Typo in method parameter name: searcRequest instead of searchRequest. This will compile but makes the code harder to read and maintain.

private Task extractParentTask(final SearchRequest searcRequest) {
Null Pointer Risk

If taskManager.getTask(taskId.getId()) returns null (e.g., task expired or not found), the method returns null, and this null is passed to resolvePipeline. While ProcessorGenerationContext accepts @Nullable String parentAction, there is no explicit handling if the task lookup fails. If processors rely on a non-null parent action when a taskId is present, this could cause unexpected behavior. Consider logging or documenting this scenario.

private Task extractParentTask(final SearchRequest searcRequest) {
    TaskId taskId = searcRequest.getParentTask();
    if (taskId != null && taskId != TaskId.EMPTY_TASK_ID) {
        return taskManager.getTask(taskId.getId());
    }
    return null;
}

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 684e48c
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Fix parameter name typo

Fix the typo in the parameter name searcRequest which should be searchRequest. This
typo could cause confusion and reduce code readability.

server/src/main/java/org/opensearch/action/search/TransportSearchAction.java [520-526]

-private Task extractParentTask(final SearchRequest searcRequest) {
-    TaskId taskId = searcRequest.getParentTask();
+private Task extractParentTask(final SearchRequest searchRequest) {
+    TaskId taskId = searchRequest.getParentTask();
     if (taskId != null && taskId != TaskId.EMPTY_TASK_ID) {
         return taskManager.getTask(taskId.getId());
     }
     return null;
 }
Suggestion importance[1-10]: 9

__

Why: The typo searcRequest instead of searchRequest is a clear error that should be fixed. This affects code readability and consistency, though it doesn't impact functionality since the parameter is still used correctly throughout the method.

High
Handle potential null task return

The taskManager.getTask() call may return null if the task is not found. Consider
handling this case explicitly or documenting that null is an expected return value
when the task doesn't exist.

server/src/main/java/org/opensearch/action/search/TransportSearchAction.java [520-526]

-private Task extractParentTask(final SearchRequest searcRequest) {
-    TaskId taskId = searcRequest.getParentTask();
+private Task extractParentTask(final SearchRequest searchRequest) {
+    TaskId taskId = searchRequest.getParentTask();
     if (taskId != null && taskId != TaskId.EMPTY_TASK_ID) {
-        return taskManager.getTask(taskId.getId());
+        Task task = taskManager.getTask(taskId.getId());
+        // Returns null if task not found, which is handled by caller
+        return task;
     }
     return null;
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion adds a comment about null handling but doesn't change the actual behavior. The method already returns null when the task is not found, and the 'improved_code' is functionally identical to the 'existing_code' (aside from fixing the typo). This is more of a documentation suggestion than a functional improvement.

Low

Previous suggestions

Suggestions up to commit d02e58d
CategorySuggestion                                                                                                                                    Impact
General
Fix parameter name typo

Fix the typo in the parameter name searcRequest which should be searchRequest. This
typo could cause confusion and reduce code readability.

server/src/main/java/org/opensearch/action/search/TransportSearchAction.java [520-526]

-private Task extractParentTask(final SearchRequest searcRequest) {
-    TaskId taskId = searcRequest.getParentTask();
+private Task extractParentTask(final SearchRequest searchRequest) {
+    TaskId taskId = searchRequest.getParentTask();
     if (taskId != null && taskId != TaskId.EMPTY_TASK_ID) {
         return taskManager.getTask(taskId.getId());
     }
     return null;
 }
Suggestion importance[1-10]: 9

__

Why: The typo searcRequest instead of searchRequest is a clear error that should be fixed. This affects code readability and consistency, and could lead to confusion during code maintenance.

High
Handle potential null from getTask

The taskManager.getTask() method may return null if the task is not found. Consider
adding a null check or documenting this behavior to prevent potential
NullPointerExceptions in calling code that expects a non-null Task when taskId is
valid.

server/src/main/java/org/opensearch/action/search/TransportSearchAction.java [520-526]

-private Task extractParentTask(final SearchRequest searcRequest) {
-    TaskId taskId = searcRequest.getParentTask();
+private Task extractParentTask(final SearchRequest searchRequest) {
+    TaskId taskId = searchRequest.getParentTask();
     if (taskId != null && taskId != TaskId.EMPTY_TASK_ID) {
-        return taskManager.getTask(taskId.getId());
+        Task task = taskManager.getTask(taskId.getId());
+        if (task == null) {
+            logger.debug("Parent task with id {} not found", taskId.getId());
+        }
+        return task;
     }
     return null;
 }
Suggestion importance[1-10]: 5

__

Why: While adding null handling for taskManager.getTask() could be beneficial, the current implementation already returns null when the task is not found, which is a valid pattern. The suggestion adds logging but doesn't fundamentally change the behavior or fix a critical issue.

Low

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for d02e58d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 684e48c

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 684e48c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

}

private Task extractParentTask(final SearchRequest searcRequest) {
TaskId taskId = searcRequest.getParentTask();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: searchRequest.getParentTask

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will change

try (
Engine.GetResult get = indexShard.get(
new Engine.Get(realtime, true, id, uidTerm).version(version)
new Engine.Get(realtime, true, id, uidTerm)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Not needed for this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will revert

}
// Resolve system generated search pipeline
final Map<String, Object> config = Map.of(SEARCH_REQUEST, searchRequest);
final String parentAction = parentTask != null ? parentTask.getAction() : null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we use structured task types here instead of raw strings? I was thinking of going one step ahead and adding in the necessary details instead of just the task name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea is to limit access to whats required. Having said that we can add type, id is available from searchRequest. I am not sure if requestHeaders are needed

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

Labels

enhancement Enhancement or improvement to existing feature or request Search Search query, autocomplete ...etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC] Expose Parent Action Name in System Generated Processor Context

2 participants