Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Oct 5, 2024

Conversation

imvtsl
Copy link
Contributor

@imvtsl imvtsl commented Sep 28, 2024

Signed-off-by: Vatsal vatsal.v.anand@gmail.com

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
Copy link
Contributor Author

imvtsl commented Sep 28, 2024

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
Copy link
Member

dblock commented Sep 30, 2024

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
Copy link
Contributor Author

imvtsl commented Oct 1, 2024

@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.

@imvtsl imvtsl marked this pull request as ready for review October 1, 2024 02:18
dblock
dblock previously approved these changes Oct 1, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
@imvtsl
Copy link
Contributor Author

imvtsl commented Oct 2, 2024

Rebase done.

Copy link
Contributor

github-actions bot commented Oct 2, 2024

Changes Analysis

Commit SHA: c7bae5d
Comparing To SHA: 939a292

API Changes

Summary

├─┬Paths
│ ├─┬/{index}
│ │ └─┬DELETE
│ │   └─┬Responses
│ │     └──[➕] codes (27887:7)
│ └─┬/{index}/_doc/{id}
│   ├─┬GET
│   │ └─┬Responses
│   │   └──[➕] codes (27727:7)
│   └─┬DELETE
│     └─┬Responses
│       └──[➕] codes (27439:7)
└─┬Components
  ├──[➕] responses (27887:7)
  ├──[➕] responses (27727:7)
  ├──[➕] responses (27439:7)
  ├──[➕] schemas (31347:7)
  └──[➕] schemas (47307: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/11189230449/artifacts/2019049822

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

Copy link
Contributor

github-actions bot commented Oct 2, 2024

Spec Test Coverage Analysis

Total Tested
504 303 (60.12 %)

@dblock
Copy link
Member

dblock commented Oct 2, 2024

Looks like some more lint/validate failures, thanks!

Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
@dblock
Copy link
Member

dblock commented Oct 3, 2024

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

Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
@imvtsl
Copy link
Contributor Author

imvtsl commented Oct 3, 2024

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.

@@ -2837,11 +2841,12 @@ components:
creation_time:
type: integer
format: int64
delete@200:
delete@200: &WriteResponseBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock
Copy link
Member

dblock commented Oct 4, 2024

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?

Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
@imvtsl
Copy link
Contributor Author

imvtsl commented Oct 5, 2024

I removed anchors to maintain consistency.

open an issue to edit everything not to?

I don't really follow this part. Could you help elaborate? Thanks.

@dblock
Copy link
Member

dblock commented Oct 5, 2024

open an issue to edit everything not to?

I don't really follow this part. Could you help elaborate? Thanks.

I mean if you think it's better/cleaner/faster/prettier to use anchors then you can suggest we change to use them everywhere in a new issue. We value your opinion and we can discuss the pros/cons on GitHub.

Thanks for your help with this!

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spotted a grammar thing, sorry to be annoying! I promise that's it after that one :)

tests/default/indices/doc.yaml Outdated Show resolved Hide resolved
Signed-off-by: Vatsal <vatsal.v.anand@gmail.com>
@imvtsl
Copy link
Contributor Author

imvtsl commented Oct 5, 2024

I mean if you think it's better/cleaner/faster/prettier to use anchors then you can suggest we change to use them everywhere in a new issue.

I will analyze yaml files in this repo and check if we should use anchors and create issue.

Spotted a grammar thing, sorry to be annoying!

No worries!

@imvtsl imvtsl requested a review from dblock October 5, 2024 01:26
@dblock dblock merged commit 118be4f into opensearch-project:main Oct 5, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants