search

CenturyLinkCloud / clc-net-sdk

Apache License 2.0
5 stars 2 forks source link

Useful "404 Not Found" response is concealed from consumer #3

Open Tolk-Haggard opened 9 years ago

Tolk-Haggard commented 9 years ago

The ServiceInvoker unconditionally converts all 404 responses to the default return type (eg. 0, false, null) and in the worst case could be misleading but is at least masking potentially important feedback (eg. the server they are requesting does not exist because of a typo in their code).

A comment in the code does attempt to explain this behavior but it still seems concerning (ie. are we trying to cover up an issue that should be corrected in the v2 APIs?): "This logic is necessary to prevent exceptions from being thrown when a resource is not found due to bad data in the system."

rpierry commented 9 years ago

We found instances where Groups/Data Centers would contain links to servers that no longer existed. I think you're right that the best course is to find and fix any data or API problems, but until that is done this seems like a reasonable approach.

Given that we're navigating internal links (vs building URLs), the exposed behavior to clients is just that things that return 404 don't exist, which is as expected.

Tolk-Haggard commented 9 years ago

Were the instances where Groups/Data Centers contain links to servers that no longer existed a frequent occurrence? And were the non-existent servers recent deletions (race condition) or just bad data somewhere?

I agree suppressing the 404s is better than regularly failing Group or Data Center actions because of stale data. Is it worth keeping this issue around until the bad data issues are resolved in the V2 APIs as a reminder to reevaluate 404 handling in the future?

rpierry commented 9 years ago

They were frequent enough, though it could have been our test accounts. I don't think it was a race condition.

I think it makes sense to keep around.