Fix painless casting bug causing opensearch to crash

[shiv0408]

Description

Update

Throwing ClassCastException when trying to cast def to void.
Created a Unit Test to ensure that def to void casting throws ClassCastException. Removed the following mention Unit Test as it was just added to reproduce the issue. The current unit test should be enough to avoid any regression in future.

Created a unit test to reproduce the bug #6435 by creating a unit test. Fix for this is requires a small change in modules/lang-painless/src/main/java/org/opensearch/painless/AnalyzerCaster.java to throw ClassCastException when we are trying to cast def to void. But to create this unit test, I had to make change supported scripts in _scripts/painless/_execute API by adding the update context. How can we keep this unit test in this code without modifying the supported scripts? One option is we can keep a setting which users can't modify and set it from the test, using that setting allow the update operation in supported scripts.

Related Issues

Resolves #6435

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/18562/
• CommitID: 4c711f0
  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?

[dblock]

I had to make change supported scripts in _scripts/painless/_execute API by adding the update context

So this is a new feature? I don't know this code well, what does this do? What are the downsides? If it's new, you should make a PR adding the feature first, then another one that fixes the bug we're talking about on top, just to keep things clean.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/18681/
• CommitID: d32fc5f
  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?

[shiv0408]

So this is a new feature?

This change would allow users to run painless scripts with update context. Although, it is not my intention to add this as a new feature. Rather than creating a new feature, I wanted to keep context which is only used in tests and not by customers.

what does this do? What are the downsides?

This API allows customers to execute painless scripts with are not stored. Providing an Update context script has no major use case here.

[reta]

@shiv0408 could you please clarify what is supposed outcome of this script (from the issue):

curl -X POST "localhost:9200/hockey/_update/3?pretty" -H 'Content-Type: application/json' -d'
{
  "script": {
    "source": "def x=1;return x;",
    "lang": "painless",
    "params": {
      "sold_cost": 26
    }
  }
}

• the sold_cost is not used - why it is there?
• the ctx._source is not used - nothing to update?

[1] https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-update.html#update-api-example

[shiv0408]

@reta I was modifying some script to reproduce the issue. The purpose of this script is just to demonstrate the issue, so did not use ctx._source to update any doc and param is also not required, left it by mistake.

[shiv0408]

And I am calling this update context, but any void context like ingest or reindex can face same issue if we try to return def type in a void expecting context.

[reta]

And I am calling this update context, but any void context like ingest or reindex can face same issue if we try to return def type in a void expecting context.

So if the context is void expected - why script should return anything? And what is happening with the results that script returns? (as I see here, we just drop it)

[shiv0408]

Thanks for approval @reta. Added DCO on all commits.

[reta]

@dblock looks good to you?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
      1 org.opensearch.indices.replication.SegmentReplicationTargetServiceTests.testOnNewCheckpointFromNewPrimaryCancelOngoingReplication
      1 org.opensearch.indices.replication.SegmentReplicationTargetServiceTests.classMethod

• URL: https://build.ci.opensearch.org/job/gradle-check/19577/
• CommitID: 14a25d3
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[shiv0408]

@reta Can we try to get this merged before 2.9 branch cut?

[reta]

@reta Can we try to get this merged before 2.9 branch cut?

@dblock had comments / concerns, we need his approval

[codecov]

Codecov Report

Merging #8315 (c4519e0) into main (397069f) will increase coverage by 0.52%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #8315      +/-   ##
============================================
+ Coverage     70.77%   71.30%   +0.52%     
- Complexity    56893    57300     +407     
============================================
  Files          4758     4758              
  Lines        269417   269417              
  Branches      39420    39420              
============================================
+ Hits         190679   192095    +1416     
+ Misses        62635    61479    -1156     
+ Partials      16103    15843     -260     

Impacted Files	Coverage Δ
...n/java/org/opensearch/painless/AnalyzerCaster.java	76.81% <100.00%> (ø)

... and 445 files with indirect coverage changes

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/19755/
• CommitID: b73ae52

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/19756/
• CommitID: c4519e0

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

• URL: https://build.ci.opensearch.org/job/gradle-check/19757/
• CommitID: c4519e0
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[shiv0408]

@dblock I think it should be good to merge now.

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-8315-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a39f60f482c405a102e6eec83097e91acc90a889
# Push it to GitHub
git push --set-upstream origin backport/backport-8315-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-8315-to-2.x.