Throw proper exception to invalid k-NN query

[junqiu-lei]

Description

Throw proper exception to invalid k-NN query in some cases:

1. In case of field “vector” is missing, it will cause NullPointerException;
2. In case of the array of “vector” contains non-number object, it will cause cast exception;

After the PR, the response will be:

// Case 1
{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "[knn] requires 'vector' to be non-null"
            }
        ],
        "type": "illegal_argument_exception",
        "reason": "[knn] requires 'vector' to be non-null"
    },
    "status": 400
}

// Case 2
{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "[knn] requires 'vector' to be an array of numbers"
            }
        ],
        "type": "illegal_argument_exception",
        "reason": "[knn] requires 'vector' to be an array of numbers"
    },
    "status": 400
}

Issues Resolved

Closes #1379

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed as per the DCO using --signoff

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.

[heemin32]

I would prefer to keep the IT tests to be minimal by not including error cases.
If the test can be covered in unit test KNNQueryBuilderTests, can we remove tests in VectorDataTypeIT?

[navneet1v]

I would say its better have the IT and unit tests. Both have their use. So, I would like to keep both of them.

[junqiu-lei]

I'd like to keep the IT cases here, it's still a potential behavior from customers, it's good to have the IT to verify from end to end.

[heemin32]

What is the case where unit test fails to catch but IT can catch here?
If the issue can be caught using simple unit test, why do we want to add expensive/duplicated IT test?

[junqiu-lei]

@heemin32 The unit test cannot verify the return response code which is expected to be 400(BAD_REQUEST), It can be safer to make sure the right response code is returned to end user by IT in k-NN plugin level. Ref: https://github.com/opensearch-project/OpenSearch/blob/2de44a7b771c9b8b59f57069d0fdfdf9ee818ec2/libs/core/src/main/java/org/opensearch/ExceptionsHelper.java#L99-L100

[navneet1v]

@heemin32

What is the case where unit test fails to catch but IT can catch here?

The case where someone wraps another exception after k-NN has thrown the excpetion.

The responsibility of return code is not in your code but in the OpenSearch framework. All you need to do is throwing correct exception.

Yes responsibility lies with Opensearch to send the correct status code, but we are relying on a behavior of Opensearch and as this exception is thrown from k-NN side we need to make sure that our customers are getting right status code. Otherwise we could have created a new exception of our own.

A simple understanding of adding an IT here is, now on the rest layer a different response will be returned hence we need to make sure that same response code is received by customer what we are expecting it to receive, and in this case that is 400.

Also, having more tests is always better. I am not able to understand why there is harm is having an IT which tests negative scenarios for k-NN.

[heemin32]

The case where someone wraps another exception after k-NN has thrown the excpetion.

1. The case is rare. 2. If this is high concern, we need a better mechanism to prevent it rather than relying on integration test.

Having more tests is always better.

Yes. However, it is not free but it comes with costs: longer test time, engineering efforts on implementation and maintenance.

[navneet1v]

If this is high concern, we need a better mechanism to prevent it rather than relying on integration test.

as now integration test is the best way. Another thing is the change which is being done in this PR is changing the RestStatus code, so having a IT is must for cases like this.

[heemin32]

I don't think OS core will do anything to change the behavior on existing exception. If this is concern we need to have our own exception and code can be verified using unit test.

If the concern is someone wrap exception inside knn repo, we can also validate it by writing a unit test on the most outer method.

[junqiu-lei]

Thanks all of you on the feedbacks. I've kept the integration tests. They offer a broader check against system-wide issues that unit tests might miss. We'll strive for a balance between comprehensive testing and efficiency.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (271df52) 85.16% compared to head (76e7680) 85.06%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1380      +/-   ##
============================================
- Coverage     85.16%   85.06%   -0.11%     
  Complexity     1258     1258              
============================================
  Files           163      163              
  Lines          5110     5114       +4     
  Branches        479      480       +1     
============================================
- Hits           4352     4350       -2     
- Misses          553      559       +6     
  Partials        205      205              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[opensearch-trigger-bot]

The backport to 2.x failed:

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

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
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1380-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 45282e056a9eb875aa53b08f7ee5865b277e651b
# Push it to GitHub
git push --set-upstream origin backport/backport-1380-to-2.x
# Go back to the original working tree
cd ../..
# 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-1380-to-2.x.