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

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

Open
Jakob3xD opened this issue Sep 12, 2023 · 11 comments
Open

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

Jakob3xD opened this issue Sep 12, 2023 · 11 comments
Assignees
Labels
API Issues with external APIs enhancement Enhancement or improvement to existing feature or request Indexing & Search RFC Issues requesting major changes

Comments

@Jakob3xD
Copy link

Jakob3xD commented Sep 12, 2023

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 opensearch-project/opensearch-plugins#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
Copy link
Member

mgodwan commented Jul 22, 2024

@shwetathareja @Bukhtawar Thoughts on this?

@dblock
Copy link
Member

dblock commented Jul 22, 2024

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

imvtsl commented Jul 23, 2024

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

@peternied peternied added the RFC Issues requesting major changes label Jul 24, 2024
@vikasvb90
Copy link
Contributor

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

@vikasvb90 vikasvb90 added the API Issues with external APIs label Aug 5, 2024
@imvtsl
Copy link
Contributor

imvtsl commented Aug 7, 2024

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

dblock commented Aug 7, 2024

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

@imvtsl
Copy link
Contributor

imvtsl commented Aug 8, 2024

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

dblock commented Aug 12, 2024

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

imvtsl commented Aug 13, 2024

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

imvtsl commented Sep 16, 2024

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

@dblock
Copy link
Member

dblock commented Sep 19, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Issues with external APIs enhancement Enhancement or improvement to existing feature or request Indexing & Search RFC Issues requesting major changes
Projects
Status: 🆕 New
Development

No branches or pull requests

8 participants