Add 404 spec for delete index, get document, delete document APIs

[imvtsl]

Signed-off-by: Vatsal [email protected]

Description

Added 404 spec for below APIs

1. DELETE /{index}
2. GET /{index}/_doc/{id}
3. DELETE /{index}/_doc/{id}

Issues Resolved

Contributes to #445

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.

[imvtsl]

Hi @dblock

Doc tests are failing for GET and DELETE /{index}/_doc/{id} in case of 404:

[INFO] => GET /movies/_doc/1 ({}) [application/json] 
[INFO] <= 404 (application/json) | Not Found
[INFO] # {
  "$ref": "#/components/schemas/_core.get:GetResult",
  "type": "object",
  "unevaluatedProperties": {
    "not": true,
    "errorMessage": "property is not defined in the spec"
  }
}
[INFO] * undefined
[INFO] & "data must be object, data must be object"
[INFO] => DELETE /movies/_doc/1 ({}) [application/json] 
[INFO] <= 404 (application/json) | Not Found
[INFO] # {
  "$ref": "#/components/schemas/_common:WriteResponseBase",
  "type": "object",
  "unevaluatedProperties": {
    "not": true,
    "errorMessage": "property is not defined in the spec"
  }
}
[INFO] * undefined
[INFO] & "data must be object, data must be object"


FAILED  doc.yaml (tests/default/indices/doc.yaml)
    FAILED  CHAPTERS
        PASSED  Create a document.
            PASSED  PARAMETERS
                PASSED  index
            PASSED  REQUEST BODY
            PASSED  RESPONSE STATUS
            PASSED  RESPONSE PAYLOAD BODY
            PASSED  RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES
        PASSED  Create a document.
            PASSED  PARAMETERS
                PASSED  index
                PASSED  id
            PASSED  REQUEST BODY
            PASSED  RESPONSE STATUS
            PASSED  RESPONSE PAYLOAD BODY
            PASSED  RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES
        PASSED  Update a document.
            PASSED  PARAMETERS
                PASSED  index
                PASSED  id
            PASSED  REQUEST BODY
            PASSED  RESPONSE STATUS
            PASSED  RESPONSE PAYLOAD BODY
            PASSED  RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES
        PASSED  Retrieve a document.
            PASSED  PARAMETERS
                PASSED  index
                PASSED  id
            PASSED  REQUEST BODY
            PASSED  RESPONSE STATUS
            PASSED  RESPONSE PAYLOAD BODY
            PASSED  RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES
        PASSED  Delete a document.
            PASSED  PARAMETERS
                PASSED  index
                PASSED  id
            PASSED  REQUEST BODY
            PASSED  RESPONSE STATUS
            PASSED  RESPONSE PAYLOAD BODY
            PASSED  RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES
        FAILED  Retrieve a non existing document.
            PASSED  PARAMETERS
                PASSED  index
                PASSED  id
            PASSED  REQUEST BODY
            PASSED  RESPONSE STATUS
            PASSED  RESPONSE PAYLOAD BODY
            FAILED  RESPONSE PAYLOAD SCHEMA (data must be object, data must be object)
            SKIPPED RESPONSE OUTPUT VALUES
        FAILED  Delete a non existing document.
            PASSED  PARAMETERS
                PASSED  index
                PASSED  id
            PASSED  REQUEST BODY
            PASSED  RESPONSE STATUS
            PASSED  RESPONSE PAYLOAD BODY
            FAILED  RESPONSE PAYLOAD SCHEMA (data must be object, data must be object)
            SKIPPED RESPONSE OUTPUT VALUES
    PASSED  EPILOGUES
        PASSED  DELETE /movies

It gives error "(data must be object, data must be object)", "errorMessage": "property is not defined in the spec"
However, GET and DELETE /{index}/_doc/{id} with 200 are PASSED and they use same schema as 404. So, I am not sure why we get this error. What am I missing? Can you help me fix this? This is my first contribution to this repo, appreciate the help. Thanks.

[dblock]

This error means that for a deleted document there's no data returned, however we can see that it's not true.

$ curl -s -k -u admin:$OPENSEARCH_PASSWORD  -X POST https://localhost:9200/movies/_doc/1 --json {} | jq
{
  "_index": "movies",
  "_id": "1",
  "_version": 2,
  "result": "updated",
  "_shards": {
    "total": 2,
    "successful": 1,
    "failed": 0
  },
  "_seq_no": 1,
  "_primary_term": 1
}

$ curl -s -k -u admin:$OPENSEARCH_PASSWORD  -X DELETE https://localhost:9200/movies/_doc/1 | jq
{
  "_index": "movies",
  "_id": "1",
  "_version": 3,
  "result": "deleted",
  "_shards": {
    "total": 2,
    "successful": 1,
    "failed": 0
  },
  "_seq_no": 2,
  "_primary_term": 1
}

$ curl -s -k -u admin:$OPENSEARCH_PASSWORD  -X GET https://localhost:9200/movies/_doc/1 | jq
{
  "_index": "movies",
  "_id": "1",
  "found": false
}

Looking at the code we do something funky with the errors here: https://github.com/opensearch-project/opensearch-api-specification/blob/main/tools/src/tester/ChapterReader.ts#L65. If the response contains a JSON error we return it, otherwise we return nothing, so this explains why the response is nothing. This looks like a non-standard 404 response.

Here's a quick fix.

diff --git a/tools/src/tester/ChapterReader.ts b/tools/src/tester/ChapterReader.ts
index 26dd027..f85e6fd 100644
--- a/tools/src/tester/ChapterReader.ts
+++ b/tools/src/tester/ChapterReader.ts
@@ -62,7 +62,7 @@ export default class ChapterReader {
         response.status = e.response.status
         response.content_type = e.response.headers['content-type']?.split(';')[0]
         const payload = this.#deserialize_payload(e.response.data, response.content_type)
-        if (payload !== undefined) response.payload = payload.error
+        if (payload !== undefined) response.payload = payload.error ?? payload
         response.message = payload.error?.reason ?? e.response.statusText

It needs a test, feel free to add one or if it's too much work open an issue and I'll take care of it.

[imvtsl]

@dblock Thanks for quick response. Doc tests are now working with the suggested change.
I added a test for this change in ChapterReader.ts. I have limited experience with TypeScript, but I have verified the coverage and result of this new test.
Please review.

[dblock]

Looks good! Test works well. Rebase and we'll merge?

[imvtsl]

Rebase done.

[github-actions]

Changes Analysis

Commit SHA: c1039de
Comparing To SHA: 939a292

API Changes

Summary

├─┬Paths
│ ├─┬/{index}/_doc/{id}
│ │ ├─┬GET
│ │ │ └─┬Responses
│ │ │   └──[➕] codes (27727:7)
│ │ └─┬DELETE
│ │   └─┬Responses
│ │     └──[➕] codes (27439:7)
│ └─┬/{index}
│   └─┬DELETE
│     └─┬Responses
│       └──[➕] codes (27887:7)
└─┬Components
  ├──[➕] responses (27727:7)
  ├──[➕] responses (27439:7)
  ├──[➕] responses (27887:7)
  ├──[➕] schemas (47307:7)
  └──[➕] schemas (31347:7)

Document Element	Total Changes	Breaking Changes
paths	3	0
components	5	0

• Total Changes: 8
• Additions: 8

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11169183929/artifacts/2015667199

API Coverage

	Before	After	Δ
Covered (%)	588 (57.59 %)	588 (57.59 %)	0 (0 %)
Uncovered (%)	433 (42.41 %)	433 (42.41 %)	0 (0 %)
Unknown	29	29	0

[github-actions]

Spec Test Coverage Analysis

Total	Tested
504	303 (60.12 %)

[dblock]

Looks like some more lint/validate failures, thanks!

[dblock]

The validation is correct. The contents of the get@404 can be the same as get@200, but you do need 2 response objects.

[imvtsl]

I created new keys, get@404 and delete@404. However, I used anchors to reuse value from get@200 key. This way, we have two keys and we avoid duplication of values.

Let me know if we don't want to use anchors but want to duplicate values.

[dblock]

This is an interesting syntax! Where can I read about & and *?

[dblock]

Let me know if we don't want to use anchors but want to duplicate values.

The rest of the docs do not use anchors, maybe let's keep that to be consistent, and open an issue to edit everything not to?