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

500 Error Still Trying to Parse JSON Response #170

Open
z46-dev opened this issue Jan 13, 2025 · 8 comments · May be fixed by #171
Open

500 Error Still Trying to Parse JSON Response #170

z46-dev opened this issue Jan 13, 2025 · 8 comments · May be fixed by #171

Comments

@z46-dev
Copy link

z46-dev commented Jan 13, 2025

When trying to get /redfish/v1/Systems/XXXXXXXX on one of our Intel systems, an Internal Server Error is returned with the status code of 500. When using client.get(...), I would expect a None on response.dict, however, an exception is thrown. If this is intentional, why provide the response.status property. I would prefer to handle the status codes manually rather than having an exception thrown due to there being no JSON in a 500 response.

Thank you!

@mraineri
Copy link
Contributor

The primary reason looks to be more with the happy path in mind with HTTP status codes and ensure the response body is JSON-formatted. So, in the typical case where there is a response body, you want an assurance that the data is decoded into a dictionary.

However, I can see the need to harden the library a bit in this regard. There will be cases where no response body is valid.

To be clear, is your situation the service is returning a 500 with no response body, or 500 with a non-JSON response? If the latter, that should be taken back to Intel as a spec violation. All responses in the 4XX and 5XX range (if there is a body) are expected to follow the "Error responses" clause of the Redfish Spec.

@z46-dev
Copy link
Author

z46-dev commented Jan 13, 2025

The response is a 500 with no body. Thank you for your quick response!

@mraineri
Copy link
Contributor

Okay, that's good news.

I'd like to propose adding a check prior to that try/except block. If the response length is 0, return None. That will preserve existing behavior to throw an exception when an invalid payload format is returned.

@z46-dev
Copy link
Author

z46-dev commented Jan 13, 2025

Thank you, that sounds like a good solution. I think the ability to handle status codes ourselves is good too. Thank you very much!

@mraineri
Copy link
Contributor

One other consideration; could an empty dictionary be acceptable over None? I'm thinking an empty dictionary might be more compatible with others consuming the library.

@z46-dev
Copy link
Author

z46-dev commented Jan 13, 2025

That's a good question. I'm not sure how others would try to parse the response. Obviously junk data can't be provided, but an empty dict could let someone continue parsing rather than stopping when seeing None. But None also means changing the return data types which is a "semi-breaking" change. Either works for me, and it should be fine provided users query the status code first. The issue with that is we don't know if people will actively check the status code.

@mraineri
Copy link
Contributor

mraineri commented Jan 13, 2025

Thanks for your input; I'll ask around to see what's preferred.

FWIW, when I write scripts using this library, I do check the status code before accessing dictionary members. The code I'm leveraging to pull out error messages (when I get back a 4XX or 5XX) does appear to gracefully handle these cases already and should continue to work after this change (regardless of returning None or an empty dictionary).

@z46-dev
Copy link
Author

z46-dev commented Jan 13, 2025

Awesome, thank you very much. I appreciate the support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants