opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.88k stars 1.84k forks source link

[PROPOSAL] Define and use a standard way for returning 404 responses #9988

Open Jakob3xD opened 1 year ago

Jakob3xD commented 1 year ago

Is your feature request related to a problem? Please describe. Opensearch has many different user API endpoints. Some of them behave similar and some doesn't. During the refactoring of the opensearch-go lib, I notices that 404 response bodys are different between API endpoints.

Examples for some get requests. ``` GET test { "error": { "root_cause": [ { "type": "index_not_found_exception", "reason": "no such index [test]", "index": "test", "resource.id": "test", "resource.type": "index_or_alias", "index_uuid": "_na_" } ], "type": "index_not_found_exception", "reason": "no such index [test]", "index": "test", "resource.id": "test", "resource.type": "index_or_alias", "index_uuid": "_na_" }, "status": 404 } ``` ``` GET _index_template/test { "error": { "root_cause": [ { "type": "resource_not_found_exception", "reason": "index template matching [test] not found" } ], "type": "resource_not_found_exception", "reason": "index template matching [test] not found" }, "status": 404 } ``` ``` GET _scripts/test { "_id": "test", "found": false } ``` ``` GET _template/test {} ``` ``` GET _ingest/pipeline/test {} ``` ``` GET test/_explain/3 {"query":{"term":{"foo":{"value":"bar"}}}} { "_index": "test", "_id": "3", "matched": false } ```

Describe the solution you'd like I would like to see a response similar to 404 responses from index or _index_templates.

{
  "error": {
    "root_cause": [
      {
        "type": "resource_not_found_exception",
        "reason": "<resource> matching [test] not found"
      }
    ],
    "type": "resource_not_found_exception",
    "reason": "<resource> matching [test] not found"
  },
  "status": 404
}

Describe alternatives you've considered Depending on the endpoint it would also be an option to return a 200 with a body containing no results like the _nodes endpoint does. This would for example work for the _index_template and _component_template as they return an array of templates.

GET _nodes/test
{
  "_nodes": {
    "total": 0,
    "successful": 0,
    "failed": 0
  },
  "cluster_name": "fsn1-staging-opensearch1",
  "nodes": {}
}

Additional context This is a bit in relation to https://github.com/opensearch-project/opensearch-plugins/issues/215 as it is about errors but for plugins. AFAIK Opensearch has its standard way for errors but not one for when to return it.

mgodwan commented 4 months ago

@shwetathareja @Bukhtawar Thoughts on this?

dblock commented 4 months ago

I like standardization, but I wonder what the blast radius is from a change like this. OpenSearch API has a lot of inconsistencies such as this one. I would better about a big backwards incompatible change in a major release if we completed the spec in https://github.com/opensearch-project/opensearch-api-specification, then made a version of it that removes all the inconsistencies, then generated the server code from it to ensure it's always consistent.

imvtsl commented 4 months ago

I will attend triage meeting with core team tomorrow to see how to go about this.

vikasvb90 commented 3 months ago

@imvtsl / @peternied / other attendees : Can you please summarize the discussion held on this issue in the core triage meeting?

imvtsl commented 3 months ago

@imvtsl / @peternied / other attendees : Can you please summarize the discussion held on this issue in the core triage meeting?

We didn't really conclude on it. The feasibility is uncertain due to the large number of APIs and the potential impact of changing the 404 response for all of them. Additionally, we lack documentation on the current error responses for these APIs, which further complicates the assessment.

dblock commented 3 months ago

I think a big chunk of work as a prerequisite of changing any responses is https://github.com/opensearch-project/opensearch-api-specification/issues/445. It would let us actually assess what APIs behave in ways we like and which ones don't, and scope the change.

imvtsl commented 3 months ago

I can update API spec for a few index APIs to begin with. Then we can standardize index APIs. After that, we can look into other groups of APIs like search, cluster, etc? We will break this big task into groups of API. This approach is okay?

dblock commented 3 months ago

@imvtsl I think it's ok, but keep in mind that 3.0 will likely happen sometime this year (based on Lucene release schedule), so we have a limited window to make big changes.

imvtsl commented 3 months ago

@imvtsl I think it's ok, but keep in mind that 3.0 will likely happen sometime this year (based on Lucene release schedule), so we have a limited window to make big changes.

Agreed. We can try to standardize as many group of APIs as we can in 3.0. We can do remaining groups in 4.0? I can begin with index group APIs from next week.

imvtsl commented 2 months ago

I am starting on this now. Do we create sub-tasks or sub-issues for better progress tracking?

dblock commented 2 months ago

@imvtsl I wouldn't bother, maybe just keep a list of - [ ] item here.