fix(code-conversion): AbstractMigrationRecipe NPE on Null Return Type

[venetrius]

Pull Request Template

related to: #537

Description

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactoring (no functional changes)
• Test-only changes (no production code changes)

Testing Checklist

Black-Box Testing Requirements

• Tests follow black-box testing approach: verify behavior through observable outputs (logs, C8 API queries, real-world skip scenarios)
• Tests DO NOT access implementation details (DbClient, IdKeyMapper, ..impl.. packages except logging constants)
• Architecture tests pass (ArchitectureTest validates these rules)

Test Coverage

• Added tests for new functionality
• Updated tests for modified functionality
• All tests pass locally

Architecture Compliance

Run architecture tests to ensure compliance:

mvn test -Dtest=ArchitectureTest

If architecture tests fail, refactor your tests to use:

• LogCapturer for log assertions
• camundaClient.new*SearchRequest() for C8 queries
• Real-World skip scenarios (e.g., migrate children without parents)

Documentation

• Updated TESTING_GUIDELINES.md if adding new test patterns
• Added Javadoc comments for public APIs
• Updated README if user-facing changes

Checklist

• Code follows project style guidelines
• Self-reviewed the code
• Added comments for complex logic
• No new compiler warnings
• Dependent changes have been merged

Related Issues

[venetrius]

Fix NPE in AbstractMigrationRecipe When Return Type Cannot Be Resolved

Problem

The AbstractMigrationRecipe crashes with a NullPointerException when processing method invocations where the return type cannot be resolved.

java.lang.NullPointerException: Cannot invoke "String.length()" 
because "fullyQualifiedName" is null
  at JavaType$ShallowClass.build(JavaType.java:659)
  at RecipeUtils.createSimpleIdentifier(RecipeUtils.java:21)
  at AbstractMigrationRecipe$1.visitMethodInvocation(AbstractMigrationRecipe.java:389)

Root Cause

The AbstractMigrationRecipe uses OpenRewrite's cursor message system to track variable types:

1. SET: In visitVariableDeclarations, when a variable is declared with a recognized initializer, the type is stored
2. GET: In visitMethodInvocation, when accessing methods on that variable, the type is retrieved

The lookup returns null when the variable's initializer doesn't match any known migration pattern, causing the NPE.

Solution

1. Skip transformation when return type cannot be resolved
2. Add TODO comment to flag code requiring manual migration
3. Prevent duplicate comments on multiple recipe cycles

Test Cases

1. Task Query with Method Access

Task task = taskService.createTaskQuery().taskId(taskId).singleResult();
return task.getName();  // <-- NPE when visiting task.getName()

Why it fails:

• task is declared with taskService.createTaskQuery()...singleResult()
• While createTaskQuery() is a recognized precondition, the terminal method singleResult() has no BuilderReplacementSpec (only list() does), so the variable declaration is visited but no cursor message is stored
• The recipe only stores messages for variables whose initializers match a SimpleReplacementSpec or BuilderReplacementSpec
• When visiting task.getName(), the lookup for "task" returns null

After fix:

Task task = taskService.createTaskQuery().taskId(taskId).singleResult();
return //TODO: Manual migration required - could not resolve return type for: task
 task.getName();

2. Task Query Result Used in Another Query

final Task task = taskService.createTaskQuery().taskId(taskId).singleResult();
List<ProcessInstance> processInstances = runtimeService
    .createProcessInstanceQuery()
    .processInstanceId(task.getProcessInstanceId())  // <-- NPE here
    .list();

Why it fails:

• Same root cause as case 1: task is initialized via TaskQuery.singleResult() which has no BuilderReplacementSpec
• No cursor message is stored for "task"
• task.getProcessInstanceId() is visited as a ReturnReplacementSpec match, but returnTypeFqn is null
• Other transformations (e.g., ProcessInstance type change) still proceed normally

After fix:

final Task task = taskService.createTaskQuery().taskId(taskId).singleResult();
List<ProcessInstance> processInstances = runtimeService
    .createProcessInstanceQuery()
    .processInstanceId(//TODO: Manual migration required - could not resolve return type for: task
        task.getProcessInstanceId())
    .list();

3. TaskService from ProcessEngine Parameter

TaskService taskService = processEngine.getTaskService();
Task task = taskService.createTaskQuery().processInstanceBusinessKey(businessKey).singleResult();
taskService.complete(task.getId());  // <-- NPE when visiting task.getId()

Why it fails:

• taskService is initialized from processEngine.getTaskService() — this is not a direct API call that the recipe recognizes
• The recipe's matchers look for patterns like TaskService.createTaskQuery(), not ProcessEngine.getTaskService()
• task is then initialized via taskService.createTaskQuery()...singleResult() — also not matched
• No cursor message is stored for "task"
• taskService.complete(task.getId()) is transformed (it matches a known pattern), but task.getId() inside it triggers the NPE because "task" has no stored type

After fix:

TaskService taskService = processEngine.getTaskService();
Task task = taskService.createTaskQuery().processInstanceBusinessKey(businessKey).singleResult();
camundaClient
    .newCompleteUserTaskCommand(Long.valueOf(//TODO: Manual migration required - could not resolve return type for: task
        task.getId()))
    .send()
    .join();

Follow-Up: Handling More Edge Cases

This fix prevents crashes but leaves code for manual migration. The following improvements could reduce the number of skipped transformations.

Add singleResult() to BuilderReplacementSpec

Currently, list() is handled by BuilderReplacementSpec in MigrateUserTaskMethodsRecipe — the entire chain taskService.createTaskQuery()...list() is matched, the variable type is stored via cursor message, and ReturnReplacementSpec matchers (e.g., Task.getName()) work on the result.

However, singleResult() has no matching spec. There's even a commented-out singleResultMethodMatcher in MigrateUserTaskMethodsRecipe:

// Currently commented out:
static final MethodMatcher singleResultMethodMatcher =
    new MethodMatcher("org.camunda.bpm.engine.query.Query singleResult()");

What's needed:

• Add a BuilderReplacementSpec for singleResult() similar to the existing list() specs
• Map taskService.createTaskQuery()...singleResult() → C8 equivalent (e.g., search + .items().getFirst())
• Note: singleResult() returns a single Task (→ UserTask), while list() returns List<Task> (→ List<UserTask>), so the template suffix differs: .items().getFirst() vs .items()
• This would automatically enable all ReturnReplacementSpec matchers (Task.getName(), Task.getId(), etc.) to work on the result, since the cursor message would be stored

Would fix test cases: 1 (task query with method access), 2 (task query in another query)

---

Handle ProcessEngine.getTaskService()

In test case 3, TaskService is obtained from a ProcessEngine parameter. The recipe doesn't recognize processEngine.getTaskService() as a source of TaskService, so it doesn't store a cursor message.

TaskService taskService = processEngine.getTaskService();  // not matched

What's needed:

• Add a SimpleReplacementSpec or new matcher for ProcessEngine.getTaskService(), ProcessEngine.getRuntimeService(), etc.
• When matched, store the cursor message so that subsequent method calls on taskService are recognized
• Alternatively, handle this at the visitVariableDeclarations level by recognizing the return type of ProcessEngine.getXxxService() regardless of the matcher

Ambiguity:

• Requires understanding of how ProcessEngine factory methods map to C8 equivalents
• The ProcessEngine pattern is fundamentally different — in C8 there's a single CamundaClient, not separate services
• May need new spec types or visitor logic to handle "service acquisition" patterns

Would fix test case: 3 (TaskService from ProcessEngine parameter)

[Copilot]

Pull request overview

This PR fixes a NullPointerException in the AbstractMigrationRecipe class that occurred when the return type of a variable could not be resolved during code migration. The fix adds null checks and gracefully handles unresolvable types by inserting TODO comments to flag code requiring manual migration. Additionally, it corrects the Camunda 8 API method name from newUserTaskCompleteCommand to newCompleteUserTaskCommand.

Changes:

• Added null-safe handling in AbstractMigrationRecipe when returnTypeFqn is null, preventing NPE and adding TODO comments for manual migration
• Created MigrationMessages utility class to centralize migration-related comment templates
• Fixed API method name to use correct newCompleteUserTaskCommand instead of newUserTaskCompleteCommand
• Added comprehensive regression tests to verify NPE fix works correctly in multiple scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
AbstractMigrationRecipeNullReturnTypeTest.java	New regression test file with three test cases verifying that the recipe handles null return types gracefully without crashing
MigrationMessages.java	New utility class providing centralized message templates for migration comments, following the EntityConversionServiceLogs pattern
AbstractMigrationRecipe.java	Added null checks for getTypeAsFullyQualified() and returnTypeFqn, and logic to add TODO comments when return types cannot be resolved
MigrateUserTaskMethodsRecipe.java	Corrected Camunda 8 API method name from newUserTaskCompleteCommand to newCompleteUserTaskCommand

[Copilot]

Good defensive null check added to prevent NPE when getTypeAsFullyQualified returns null. This pattern should be consistently applied to all similar calls to getTypeAsFullyQualified in the codebase. Note that ReplaceTypedValueAPIRecipe.java has two similar calls on lines 294 and 461 that may benefit from the same null check pattern for consistency and robustness.

[Copilot]

This file contains a fix to the Camunda 8 API method name (changing from newUserTaskCompleteCommand to newCompleteUserTaskCommand), which is not mentioned in the PR title or description. While this is a valid and necessary correction, it should be documented in the PR description to provide complete context for reviewers. The PR description currently only mentions the NPE fix.

[HeleneW-dot]

Lgtm!