[Connector API] Null request body should result in 400 status code when body is required

[jedrazb]

Problem

Subset of Connector APIs handlers assumes that the request body is always provided. The body parameter is marked as required in the API spec (used to generate clients), but there is nothing stopping a user from sending a request without body.

If body is not provided for a request where body is expected, we are failing with a 500 error, as the server fails at processing the body that is null. This is not a good way to handle such scenario.

Changes

Looking at other API endpoint(1, 2, 3) I took inspiration and followed their pattern to gracefully handle missing body param. If body is not provided for a request that expects a body, we return informative message and 400 status code. See example:

{
    "error": {
        "root_cause": [
            {
                "type": "parse_exception",
                "reason": "request body is required"
            }
        ],
        "type": "parse_exception",
        "reason": "request body is required"
    },
    "status": 400
}

I'm using request.contentParser() - empty body would cause an exception that would be propagated as a 400 status code in the API response (like above).

I removed a bunch of unnecessary fromXContentBytes functions, in favour of keeping it simpler.

I adapted all Connector API endpoints that require the body.

Testing

• I tried adding yaml e2e tests for those scenarios but I'm getting errors from api spec if I intentionally skip body parameter in the request (so can't test this scenario there)
• No effects on existing logic, so no tests changed
• Verified locally that this works as expected

[elasticsearchmachine]

Pinging @elastic/ent-search-eng (Team:SearchOrg)

[jedrazb]

@elasticmachine merge upstream

[seanstory]

I tried adding yaml e2e tests for those scenarios but I'm getting errors from api spec if I intentionally skip body parameter in the request (so can't test this scenario there)

Would unit tests work, instead of yaml tests?

It's not clear to me (a novice) why these changes fix the issue you describe, so without test changes, I have to just take your word for it that this makes a difference. And i do trust you, but I don't trust that we won't see regressions if we don't have a way to test this.

[seanstory]

Suggested change

	restRequest.param("connector_id")
	restRequest.param(CONNECTOR_ID_PARAM)

[seanstory]

Suggested change

	restRequest.param("connector_id")
	restRequest.param(CONNECTOR_ID_PARAM)

[seanstory]

Is it conventional to define these types of constants in each action? Seems like we could re-use one constant somewhere.

[jedrazb]

Sounds like a refactoring, I filed the issue to look into this https://github.com/elastic/search-team/issues/7790 this PR got quite big already

[seanstory]

Suggested change

	restRequest.param("connector_id")
	restRequest.param(CONNECTOR_ID_PARAM)

[jedrazb]

Thank you for flagging! I forgot to run the replace-all within the module

[jedrazb]

Would unit tests work

Let me look into this!

[jedrazb]

It's not clear to me (a novice) why these changes fix the issue you describe, so without test changes, I have to just take your word for it that this makes a difference.

@seanstory You are totally right saying that, I included unit tests for all endpoints that I modified. That required adding some new files (resulting in a lot of additions), but the changes are pretty straightforward

[jedrazb]

@elasticmachine merge upstream

[seanstory]

nice!

[jedrazb]

@elasticmachine merge upstream