return parsing exception 400 for parsing errors #1593

Zhangxunmt · 2023-11-04T02:24:05Z

Description

Fix the 500 internal service error reported in the pen test. All parsing errors should return 400 error code showing it's coming from users.

Test:

PUT /_plugins/_ml/connectors/zmMCmIsBaumhjugAg9cu
{
  "description": null
}
Response:
{
  "error": {
    "root_cause": [
      {
        "type": "parse_exception",
        "reason": "Can't get text on a VALUE_NULL at 2:18"
      }
    ],
    "type": "parse_exception",
    "reason": "Can't get text on a VALUE_NULL at 2:18"
  },
  "status": 400
}

PUT /_plugins/_ml/connectors/zmMCmIsBaumhjugAg9cu
{
  "description": "Update description"
}
Response:
{
  "_index": ".plugins-ml-connector",
  "_id": "zmMCmIsBaumhjugAg9cu",
  "_version": 3,
  "result": "updated",
  "_shards": {
    "total": 1,
    "successful": 1,
    "failed": 0
  },
  "_seq_no": 2,
  "_primary_term": 2
}

Issues Resolved

[List any issues this PR will resolve]

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

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.

Signed-off-by: Xun Zhang <xunzh@amazon.com>

codecov · 2023-11-04T02:46:55Z

Codecov Report

Merging #1593 (c9aa968) into 2.x (d8e8ff2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                2.x    #1593      +/-   ##
============================================
+ Coverage     80.64%   80.66%   +0.02%     
- Complexity     4177     4178       +1     
============================================
  Files           399      399              
  Lines         16816    16818       +2     
  Branches       1815     1815              
============================================
+ Hits          13561    13566       +5     
+ Misses         2540     2537       -3     
  Partials        715      715

Flag	Coverage Δ
ml-commons	`80.66% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...pensearch/ml/rest/RestMLUpdateConnectorAction.java	`90.47% <100.00%> (+1.00%)`	⬆️

... and 1 file with indirect coverage changes

b4sjoo · 2023-11-04T08:58:41Z

plugin/src/main/java/org/opensearch/ml/rest/RestMLUpdateConnectorAction.java

+            ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser);
+            return MLUpdateConnectorRequest.parse(parser, connectorId);
+        } catch (IllegalStateException illegalStateException) {
+            throw new OpenSearchParseException(illegalStateException.getMessage());


I can confirm in line 67 IOException could also throw a 500 status code, would you mind changing the throw new IOException to throw new OpenSearchParseException?

+1 on previous comment. Don't we need to add corresponding test as well?

This is a good catch. Already updated in the new commit. The UT tests are added too.

Signed-off-by: Xun Zhang <xunzh@amazon.com>

dhrubo-os · 2023-11-05T01:24:47Z

plugin/src/test/java/org/opensearch/ml/rest/RestMLUpdateConnectorActionTests.java

@@ -114,8 +114,15 @@ public void testUpdateConnectorRequest() throws Exception {
        assertEquals("2", updateConnectorRequest.getUpdateContent().getVersion());
    }

+    public void testUpdateConnectorRequestWithParsingException() throws Exception {
+        exceptionRule.expect(OpenSearchParseException.class);
+        exceptionRule.expectMessage("Can't get text on a VALUE_NULL");


Just curious where are we setting this message? Can we have more readable message from customer's point of view?

This is the message from the XContentParser in OpenSearch, which contains the exact line and coordinate where the null comes from. exceptionRule.expectMessage only checks if the real message contains this "Can't get text on a VALUE_NULL". The means the real message returned to customer is more explanatory.

Thank you for the clarification? Just out of curiosity, do we know what's the final message we send to customer?

Yes, please check the description of the PR which has the exact coordinate of the error. Also please note this is one example of the paring exception.

return parsing exception 400 for parsing errors

cff1eb8

Signed-off-by: Xun Zhang <xunzh@amazon.com>

Zhangxunmt added the backport 2.11 label Nov 4, 2023

Zhangxunmt requested review from ylwu-amzn and b4sjoo November 4, 2023 02:24

Zhangxunmt temporarily deployed to ml-commons-cicd-env November 4, 2023 02:24 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 02:48 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 02:48 — with GitHub Actions Failure

b4sjoo reviewed Nov 4, 2023

View reviewed changes

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 23:37 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 23:37 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 23:37 — with GitHub Actions Failure

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 23:37 — with GitHub Actions Error

add more ut in restupdateconnector

c9aa968

Signed-off-by: Xun Zhang <xunzh@amazon.com>

Zhangxunmt force-pushed the 2.x branch from bc8cb38 to c9aa968 Compare November 4, 2023 23:50

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 23:50 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 4, 2023 23:50 — with GitHub Actions Failure

Zhangxunmt temporarily deployed to ml-commons-cicd-env November 4, 2023 23:51 — with GitHub Actions Inactive

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 5, 2023 00:10 — with GitHub Actions Error

Zhangxunmt had a problem deploying to ml-commons-cicd-env November 5, 2023 00:10 — with GitHub Actions Failure

dhrubo-os reviewed Nov 5, 2023

View reviewed changes

dhrubo-os approved these changes Nov 6, 2023

View reviewed changes

b4sjoo approved these changes Nov 6, 2023

View reviewed changes

Zhangxunmt merged commit c46d63c into opensearch-project:2.x Nov 6, 2023
8 of 13 checks passed

opensearch-trigger-bot bot mentioned this pull request Nov 6, 2023

[Backport 2.11] return parsing exception 400 for parsing errors #1601

Closed

Zhangxunmt mentioned this pull request Nov 6, 2023

[Backport 2.11] Rebased - return parsing exception 400 for parsing errors #1603

Merged

5 tasks

This was referenced Nov 16, 2023

Backport multiple PRs to main from 2.x #1652

Merged

Backport multiple PRs to main from 2.x #1663

Merged

Zhangxunmt mentioned this pull request Dec 15, 2023

Memory Manager and Update Memory Actions/APIs #1761

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return parsing exception 400 for parsing errors #1593

return parsing exception 400 for parsing errors #1593

Zhangxunmt commented Nov 4, 2023 •

edited by ylwu-amzn

Loading

codecov bot commented Nov 4, 2023 •

edited

Loading

b4sjoo Nov 4, 2023

dhrubo-os Nov 4, 2023

Zhangxunmt Nov 6, 2023

dhrubo-os Nov 5, 2023

Zhangxunmt Nov 6, 2023

dhrubo-os Nov 6, 2023

Zhangxunmt Nov 6, 2023

return parsing exception 400 for parsing errors #1593

return parsing exception 400 for parsing errors #1593

Conversation

Zhangxunmt commented Nov 4, 2023 • edited by ylwu-amzn Loading

Description

Issues Resolved

Check List

codecov bot commented Nov 4, 2023 • edited Loading

Codecov Report

b4sjoo Nov 4, 2023

Choose a reason for hiding this comment

dhrubo-os Nov 4, 2023

Choose a reason for hiding this comment

Zhangxunmt Nov 6, 2023

Choose a reason for hiding this comment

dhrubo-os Nov 5, 2023

Choose a reason for hiding this comment

Zhangxunmt Nov 6, 2023

Choose a reason for hiding this comment

dhrubo-os Nov 6, 2023

Choose a reason for hiding this comment

Zhangxunmt Nov 6, 2023

Choose a reason for hiding this comment

Zhangxunmt commented Nov 4, 2023 •

edited by ylwu-amzn

Loading

codecov bot commented Nov 4, 2023 •

edited

Loading