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.

Sign up for GitHub

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

Jump to bottom

[spotify] Potential wrong expiry value is used for ExpiringCache #10490

Closed
DarkC35 opened this issue Apr 8, 2021 · 1 comment · Fixed by #11370
Closed

[spotify] Potential wrong expiry value is used for ExpiringCache #10490

DarkC35 opened this issue Apr 8, 2021 · 1 comment · Fixed by #11370
Labels
bug An unexpected problem or unintended behavior of an add-on

Comments

@DarkC35
Copy link

DarkC35 commented Apr 8, 2021

Expected Behavior

I'm currently developing a new binding and use the spotify binding as a reference for the usage of the OAuth workflow.

After I inspected the SpotifyBridgeHandler code I noticed that both playingContextCache and devicesCache are using configuration.refreshPeriod as the expiry value for the cache. This refreshPeriod is described in thing-types as "Connect Refresh Period (seconds)" but ExpiringCache requires a Duration or a long value that represents the duration in milliseconds according to the JavaDoc.

I wanted to ask if this is intended (since it seems to work without spamming calls to the API every 10ms?) or if it should be changed to something like Duration.ofSeconds(configuration.refreshPeriod) like I saw in other bindings?

Current Behavior

Both devicesCache and playingContextCache are using an int value that is supposed to represent a second value as a parameter that expects a milliseconds value.

openhab-addons/bundles/org.openhab.binding.spotify/src/main/java/org/openhab/binding/spotify/internal/handler/SpotifyBridgeHandler.java

Line 290 in 1661425

playingContextCache = new ExpiringCache<>(configuration.refreshPeriod, spotifyApi::getPlayerInfo);

openhab-addons/bundles/org.openhab.binding.spotify/src/main/java/org/openhab/binding/spotify/internal/handler/SpotifyBridgeHandler.java

Line 292 in 1661425

devicesCache = new ExpiringCache<>(configuration.refreshPeriod, spotifyApi::getDevices);

Possible Solution

Use Duration.ofSeconds(configuration.refreshPeriod) instead of configuration.refreshPeriod.

Your Environment

  • Version used: 3.1.0-snapshot
@DarkC35 DarkC35 added the bug An unexpected problem or unintended behavior of an add-on label Apr 8, 2021
Hilbrand added a commit to Hilbrand/openhab-addons that referenced this issue Oct 11, 2021
@Hilbrand
[spotify] Various enhancements and fixes
eb5d0c1 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Hilbrand added a commit to Hilbrand/openhab-addons that referenced this issue Oct 11, 2021
@Hilbrand
[spotify] Various enhancements and fixes
6961b2c 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Hilbrand added a commit to Hilbrand/openhab-addons that referenced this issue Oct 11, 2021
@Hilbrand
[spotify] Various enhancements and fixes
c84ddf6 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Hilbrand added a commit to Hilbrand/openhab-addons that referenced this issue Oct 11, 2021
@Hilbrand
[spotify] Various enhancements and fixes
b4e4d13 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Hilbrand added a commit to Hilbrand/openhab-addons that referenced this issue Oct 11, 2021
@Hilbrand
[spotify] Various enhancements and fixes
61fe39f 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).
- Fixed device list update problem for devices. If no devices where configured as separate things (or the devices added where offline, but non added where not offline) the list was not updated.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
@Hilbrand Hilbrand mentioned this issue Oct 11, 2021
Merged
fwolter pushed a commit that referenced this issue Oct 24, 2021
@Hilbrand
[spotify] Various enhancements and fixes (#11370)
196e4e2 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes #6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes #10490).
- Improved handling Spotify API error messages (Closes #11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes #7400).
- Fixed device list update problem for devices. If no devices where configured as separate things (or the devices added where offline, but non added where not offline) the list was not updated.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
@openhab-bot
Copy link
Collaborator

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/spotify-connect-binding-available/66809/262

dschoepel pushed a commit to dschoepel/openhab-addons that referenced this issue Nov 9, 2021
@Hilbrand @dschoepel
[spotify] Various enhancements and fixes (openhab#11370)
a5014d9 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).
- Fixed device list update problem for devices. If no devices where configured as separate things (or the devices added where offline, but non added where not offline) the list was not updated.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Signed-off-by: Dave J Schoepel <[email protected]>
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this issue Dec 30, 2021
@Hilbrand @NickWaterton
[spotify] Various enhancements and fixes (openhab#11370)
7531a06 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).
- Fixed device list update problem for devices. If no devices where configured as separate things (or the devices added where offline, but non added where not offline) the list was not updated.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this issue Jan 28, 2022
@Hilbrand
[spotify] Various enhancements and fixes (openhab#11370)
4ebee9d 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).
- Fixed device list update problem for devices. If no devices where configured as separate things (or the devices added where offline, but non added where not offline) the list was not updated.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this issue May 5, 2022
@Hilbrand
[spotify] Various enhancements and fixes (openhab#11370)
5caa3f9 
Enhancements:
- Added play actions to start a track or other via a rule.
- Added albumImageUrl with the url to the album image to not have to send the whole image via the openHAB eventbus.
- Added a parameter imageIndex to set the image size to use (0, 1 or 2) to channels albumImage and albumImageUrl.
- Added offset and limit parameters to playlists channel (Closes openhab#6843).

Fixes:
- Fixed invalid expire value set on ExpiringCache (Closes openhab#10490).
- Improved handling Spotify API error messages (Closes openhab#11308).
- Added TTL to discovery results to fix duplicated Spotify Connect devices reported in the inbox (Closes openhab#7400).
- Fixed device list update problem for devices. If no devices where configured as separate things (or the devices added where offline, but non added where not offline) the list was not updated.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[spotify] Various enhancements and fixes Hilbrand/openhab-addons
2 participants
@openhab-bot @DarkC35