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

fix(deserializer): try-catch, to prevent parsing non deserializable responses #128

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DopiGFX
Copy link

@DopiGFX DopiGFX commented Sep 16, 2024

fix: add try-catch, to prevent parsing non deserializable responses

Problem

Requests answering empty Responses (like Pause Playback) are currently responding non-deserializable content.
This leads to a falsely thrown error.

Solution

I added a try-catch block to DefaultResponseDeserializer#deserialize to prevent the error.
I intentionally did not include logging in the catch-block because the error is falsely thrown.

Result

This only applies to Requests with empty Responses returning non-deserializable content.
It should not break any code

@DopiGFX
Copy link
Author

DopiGFX commented Sep 16, 2024

This Pull Request should resolve the following Issues: #127 #119

@ellingtonjp
Copy link

ellingtonjp commented Sep 18, 2024

Thanks for working on this! This fixes the specific issue in #127 and #119, but I think failing silently can mask future issues. For example, a future API bug could introduce malformed JSON in some other endpoint, and we'd have a harder time tracking it down since we wouldn't raise an error.

IMO this is really a problem in the API, and the API should be fixed. But in the meantime, perhaps we make it easier for clients to handle this (and future) bugs.

One option is to introduce a ResponseDeserializationError with a good error message to make it clear what's happening, and perhaps even including the response payload. This would give us the power to handle API bugs as necessary for our applications.

Thoughts?

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

Successfully merging this pull request may close these issues.

2 participants