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

[orbitbhyve] Handle null location attribute in devices json #16525

Merged
merged 2 commits into from Mar 16, 2024

Conversation

mhilbush
Copy link
Contributor

@mhilbush mhilbush commented Mar 16, 2024

Some "devices" responses contain a null value in the location attribute. Json parsing failed because JsonObject will not deserialize a null value. Also added a check for JsonSyntaxException and logged the stack trace to better be able to track down parsing issues.

BTW, I noticed that location is not used anywhere in the binding. So, an alternative would be to just remove it from the DTO. However, I still think it's a good idea to catch JsonSyntaxException and log the stack trace to be able to track down parsing issues.

This doesn't seem critical enough to cherry pick to 4.1.x.

Signed-off-by: Mark Hilbush <mark@hilbush.com>
@mhilbush mhilbush added the bug An unexpected problem or unintended behavior of an add-on label Mar 16, 2024
@mhilbush mhilbush requested a review from octa22 as a code owner March 16, 2024 01:47
@@ -210,6 +211,9 @@ public List<OrbitBhyveDevice> getDevices() {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"Get devices returned response status: " + response.getStatus());
}
} catch (JsonSyntaxException e) {
logger.info("Exception parsing devices json: {}", e.getMessage(), e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEBUG level would be more appropriate, especially if you log the full stack trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's fair. Previously before the exception was caught, it would just fail silently. Now the bridge will go offline, which will be a clue to enable debug.

Note in my latest commit I'm also catching JsonSyntaxException in the getDevice method because it will fail in the same way.

Signed-off-by: Mark Hilbush <mark@hilbush.com>
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@lolodomo lolodomo merged commit 035f446 into openhab:main Mar 16, 2024
3 checks passed
@lolodomo lolodomo added this to the 4.2 milestone Mar 16, 2024
@mhilbush mhilbush deleted the orbitbhyve-add-timer branch March 16, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants