ccsr: Fix error code for subject not found#36034
ccsr: Fix error code for subject not found#36034def- wants to merge 1 commit intoMaterializeInc:mainfrom
Conversation
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
| UnhandledError::Transport(err) => GetSubjectConfigError::Transport(err), | ||
| UnhandledError::Api { code, message } => match code { | ||
| 404 => GetSubjectConfigError::SubjectNotFound, | ||
| 40401 => GetSubjectConfigError::SubjectNotFound, |
There was a problem hiding this comment.
Are you sure "404" can never happen? In the linked docs I see some APIs that document "404 Not Found" but no sub-error codes.
There was a problem hiding this comment.
Only if the json body can't be parsed would we return the 404 error code, and would then wrongly consider it as a "SubjectNotFound". All the other error handlers in this file do it the same way.
There was a problem hiding this comment.
For example, the GET /config/{subject} API documents these errors:
Status Codes:
* 404 Not Found – Subject not found
* 500 Internal Server Error –
Error code 50001 – Error in the backend datastore
What error code does the API return for "Subject not found", assuming the response can be parsed? It looks like it's 404. Are you suggesting there is a bug in the documentation or am I missing something?
Btw, for PublishError we also match on "409", not a five-digit error code. Is that also a bug?
There was a problem hiding this comment.
From what I can tell their documentation is kind of misleading. The code seems to always set the error code to 40401 for subject not found: https://github.com/confluentinc/schema-registry/blob/master/core/src/main/java/io/confluent/kafka/schemaregistry/rest/exceptions/Errors.java#L25
The 409 is also a bug, fixing.
9eebd5d to
8741253
Compare
8741253 to
65e5f3b
Compare
See https://docs.confluent.io/platform/current/schema-registry/develop/api.html#errors for error codes
Caused by #27641
Nikhil noted this correctly in the original review: #27641 (comment)