apache / trafficcontrol

Apache Traffic Control is an Open Source implementation of a Content Delivery Network
https://trafficcontrol.apache.org/
Apache License 2.0
1.06k stars 341 forks source link

Server Capability get by name - returns blank response array when the requested item doesn't exist #4007

Open lbathina opened 4 years ago

lbathina commented 4 years ago

I'm submitting a ...

Traffic Control components affected ...

Current behavior:

response code: 200 returns

{
    "response": []
}

Expected / new behavior:

response code 404 should return alert

Text: "no server capability with that key found"
                            Level: "error"

Minimal reproduction of the problem with instructions:

When GET https://{{TO_BASE_URL}}/api/{{api_version}}/server_capabilities?name=DOESNT_EXIST where DOESNT_EXIST - is sever capability which is not in the list.

Anything else:

Also the return struct is an array of response while we expect only one to be returned.

ocket8888 commented 4 years ago

This is a matter of opinion. The URI in question does exist, it's just that a list of all the server capabilities filtered by a non-existent name is an empty list.

If a change was made to consider identifying fields special cases of filters that would cause a 404 error to be returned, it would need to be made globally, as this is the behavior of many (all?) endpoints.

I think that'd be a fine change to make - so that it would match the behavior of other request methods - but I also don't think that what we're currently doing is extremely wrong.

rob05c commented 4 years ago

I don't think this is a bug, it's a fundamental change to the API.

For a 404 vs an empty array for objects that don't exist, I don't think either is the "right" solution. A 404 is probably more expected. But to say "the API returns an array of objects, and if your filter didn't have any, you get a successful empty array" is not a violation of HTTP.

Moreover, the TO API has returned 200 [] for ages. It isn't documented behavior, but it's most certainly a specific behavior, that clients almost certainly rely on in places. Even if it doesn't violate the SemVer API Promise, we can't reasonably change this without breaking tons of people.

Likewise, IMO we should continue to make new endpoints follow the same pattern as everything else.

It wouldn't be a bad idea to do this in the next major version of the API (2.0), but we simply can't safely do it to 1.x.

mitchell852 commented 4 years ago

For what it's worth, to me this makes sense:

GET /foos/{id-does-not-exist} returns a 404 as the URI does NOT exist

and

GET /foos?id=id-does-not-exist returns an empty array as i see the query parameter as a filter of a perfectly valid URI as @ocket8888 mentioned.

mitchell852 commented 4 years ago

IMO this should simply be closed as this is expected behavior.

ocket8888 commented 4 years ago

It's not a bug but it can stay open as an enhancement request