[livetennisapi] Initial contribution - #21419
Conversation
New binding for the Live Tennis API: an account bridge (API key, shared live-match polling, usage channels) plus player and tournament things with live score, derived break-point, next-match and ranking channels. Free-tier endpoints only; defaults sized to the free quota. Signed-off-by: Ben Abulafia <ben@synapsereality.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a Live Tennis API binding for live scores, player details, tournament tracking, and API quota monitoring.
Changes:
- Implements account, player, and tournament handlers.
- Adds metadata, documentation, API DTOs, and module registration.
- Adds score-mapping and deserialization tests.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
CODEOWNERS |
Registers binding owner. |
bundles/pom.xml |
Adds binding module. |
bom/openhab-addons/pom.xml |
Adds binding dependency. |
bundles/org.openhab.binding.livetennisapi/pom.xml |
Defines module. |
bundles/org.openhab.binding.livetennisapi/NOTICE |
Adds license notice. |
bundles/org.openhab.binding.livetennisapi/README.md |
Documents configuration and channels. |
src/main/feature/feature.xml |
Defines Karaf feature. |
src/main/resources/OH-INF/addon/addon.xml |
Defines add-on metadata. |
src/main/resources/OH-INF/thing/thing-types.xml |
Defines Things, channels, and configuration. |
src/main/resources/OH-INF/i18n/livetennisapi.properties |
Adds localized labels and statuses. |
LiveTennisApiBindingConstants.java |
Defines binding identifiers. |
LiveTennisApiHandlerFactory.java |
Creates binding handlers. |
MatchStateMapper.java |
Maps scores to channel values. |
LiveTennisApiAccountConfiguration.java |
Defines account configuration. |
LiveTennisApiPlayerConfiguration.java |
Defines player configuration. |
LiveTennisApiTournamentConfiguration.java |
Defines tournament configuration. |
LiveTennisApiClient.java |
Implements API requests. |
LiveTennisApiException.java |
Defines base API exception. |
LiveTennisApiAuthenticationException.java |
Defines authentication failure. |
LiveTennisApiNotFoundException.java |
Defines missing-resource failure. |
Match.java |
Models matches. |
MatchListResponse.java |
Models match-list responses. |
MatchPlayers.java |
Models participants. |
Player.java |
Models players. |
Score.java |
Models score snapshots. |
Tournament.java |
Models tournaments. |
Usage.java |
Models quota usage. |
LiveMatchesListener.java |
Defines snapshot listener contract. |
LiveTennisApiAccountHandler.java |
Polls and distributes live data. |
LiveTennisApiPlayerHandler.java |
Updates player channels. |
LiveTennisApiTournamentHandler.java |
Updates tournament channels. |
MatchStateMapperTest.java |
Tests score mapping. |
DeserializationTest.java |
Tests API deserialization. |
matches-live.json |
Provides match test data. |
usage.json |
Provides usage test data. |
Suppressed comments (3)
bundles/org.openhab.binding.livetennisapi/src/main/java/org/openhab/binding/livetennisapi/internal/handler/LiveTennisApiAccountHandler.java:135
- A failure in the auxiliary
/usagerequest currently discards an already successful live-match response, skips child notifications, and marks the bridge offline. Since/usagecan independently fail or be rate-limited, publish the core live snapshot first and refresh usage in a separate failure boundary.
List<Match> liveMatches = client.getLiveMatches();
Usage usage = client.getUsage();
lastLiveMatches = liveMatches;
bundles/org.openhab.binding.livetennisapi/src/main/java/org/openhab/binding/livetennisapi/internal/handler/LiveTennisApiPlayerHandler.java:162
- A detail request can succeed while the bridge is
OFFLINEbecause the bridge retains its API client after polling failures. This call then overwrites theBRIDGE_OFFLINEstatus set bysuper.bridgeStatusChanged, reporting the player online without a successful live snapshot. Leave the ONLINE transition toonLiveMatches(), which runs after a successful bridge poll.
setOnlineUnlessMisconfigured();
bundles/org.openhab.binding.livetennisapi/src/main/java/org/openhab/binding/livetennisapi/internal/handler/LiveTennisApiTournamentHandler.java:134
- A metadata request can succeed while the bridge is
OFFLINEbecause the bridge retains its API client after polling failures. This call then overwritesBRIDGE_OFFLINE, reporting the tournament online without a successful live snapshot. Leave the ONLINE transition toonLiveMatches(), which runs after a successful bridge poll.
setOnlineUnlessMisconfigured();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| MatchListResponse response = get("/matches?status=live&limit=" + LIVE_MATCH_LIMIT, MatchListResponse.class); | ||
| List<Match> matches = response.data; | ||
| return matches == null ? List.of() : matches; |
There was a problem hiding this comment.
The warning does not resolve the correctness issue. The API explicitly provides meta.has_more, while MatchListResponse still discards meta.
Since child handlers interpret this list as a complete snapshot, matches beyond the first page are incorrectly treated as absent. Pagination should normally be handled transparently. If fetching every page is undesirable because additional pages consume the API quota, a configurable page/request limit could be considered, but reaching that limit must result in an explicitly incomplete snapshot rather than treating the first page as authoritative.
There was a problem hiding this comment.
Normal multi-page pagination is fixed, but the completeness issue still exists at the five-page cap. If meta.has_more is still true after page five, collectLiveMatches() logs the truncation and returns the partial list, after which the child handlers interpret it as a complete snapshot.
If the page cap is intentionally kept as a defensive bound, reaching it should result in an explicitly incomplete/failed poll rather than publishing the truncated list as authoritative. The README should also not say that the bridge pages until the snapshot is complete while this case exists.
There was a problem hiding this comment.
After restoring the client source, this still remains unresolved. The intended latest change is documentation-only; collectLiveMatches() from the previous revision still logs at the five-page cap and returns the partial all list. Child handlers consume that list as an authoritative snapshot, so tracked entities beyond the cap can still be cleared or undercounted. Reaching the cap should propagate an incomplete/failed snapshot rather than a partial snapshot being treated as complete.
| ScheduledFuture<?> job = pollingJob; | ||
| if (job != null) { | ||
| job.cancel(true); | ||
| pollingJob = null; | ||
| } | ||
| apiClient = null; |
There was a problem hiding this comment.
The added disposed guard still does not make this safe across reconfiguration. BaseThingHandler reuses the same handler instance by calling dispose() followed by initialize(), and initialize() sets disposed back to false.
An API request that started before dispose() can therefore finish after reinitialization, observe disposed == false, and publish cache/status updates belonging to the previous configuration.
Please use a lifecycle generation/token captured by each poll and only publish results when that generation still matches the current handler lifecycle.
There was a problem hiding this comment.
The generation check prevents the old refresh from publishing, but the shared refreshInProgress flag creates a reconfiguration race in the player handler. A refresh started by the untracked scheduler.execute() can remain in flight while dispose() / initialize() runs. The new lifecycle's five-second scheduled refresh then returns because the old invocation still holds the flag; once the old invocation exits, nothing retriggers the refresh until the next detail interval (7200 seconds by default). Please make the in-progress guard lifecycle-aware or reschedule the current generation when it loses this race.
| if (bridgeStatusInfo.getStatus() == ThingStatus.ONLINE) { | ||
| scheduler.execute(this::refreshDetails); | ||
| } |
There was a problem hiding this comment.
This still applies. The reconnect-triggered refreshDetails() can overlap the periodic detail job, and on normal startup the bridge can become ONLINE before the five-second initial detail delay expires, causing the same two counted player-detail requests to run twice within a few seconds. Using a single scheduling path, or rescheduling/skipping when details were just refreshed, would avoid unnecessary quota use in addition to addressing the lifecycle issue.
There was a problem hiding this comment.
The refreshInProgress guard prevents concurrent refreshes, but it does not prevent near-sequential duplicate requests.
On startup the periodic detail refresh is already scheduled for five seconds later, while the bridge becoming ONLINE immediately executes refreshDetails(). If that first refresh finishes before the scheduled task runs, another two counted requests are still made a few seconds later.
A pending transient retry can similarly survive a successful ONLINE-triggered refresh and run afterward. Using one scheduling path, or cancelling/rescheduling pending periodic and retry work after a successful immediate refresh, would avoid these unnecessary quota bursts.
| tournamentId = config.tournamentId; | ||
| updateStatus(ThingStatus.UNKNOWN); | ||
|
|
||
| scheduler.execute(this::refreshInfo); |
lsiepel
left a comment
There was a problem hiding this comment.
Noticed this is your first contribution. Welcome to the openHAB project.
Thanks for taking the time and effort to do so.
Looked at all files. Pretty solid, many small findings, probably need another round when these are addressed.
| /** | ||
| * A player (or doubles team) as returned by the Live Tennis API. | ||
| * | ||
| * @author Ben - Initial contribution |
There was a problem hiding this comment.
- @author Ben - Initial contribution
It should also state your last name
There was a problem hiding this comment.
The rewritten commits now identify the contributor as Ben Abulafia. The repository guidance requires @author to use the contributor's real human name rather than an alias, so Ben Synapse should be changed to Ben Abulafia across the new Java sources.
There was a problem hiding this comment.
The latest commit only updated this in part of the binding. Current sources still contain @author Ben Synapse, for example LiveTennisApiBindingConstants, so this thread remains unresolved. Please apply Ben Abulafia consistently across all new Java sources.
Review round with lsiepel and wborn: - Annotate all DTO fields with @nullable and document that a doubles team is one participant (pairing as name, per-individual bio null), not two merged players. - Add doubles support surfaced as a live#discipline channel (singles/ doubles) on the player and tournament things, backed by the honest three-valued draw field; add a doubles deserialization test. - Add @author surname across the binding. - Use wildcard static imports for the binding constants. - Map HTTP 429 and request timeouts to a LiveTennisApiTransientException so the player and tournament handlers schedule a backoff retry rather than waiting for the next scheduled cycle. - Hide the API client behind the bridge handler: children fetch through fetchPlayer/fetchNextMatch/fetchTournament instead of holding the client, gate detail fetches on an ONLINE bridge, and leave the ONLINE transition to the bridge poll; track and cancel the retry/info jobs. - Reflow thing-types.xml via spotless and align the README tables. - Warn when the live snapshot hits the page limit. Signed-off-by: Ben Abulafia <ben@synapsereality.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks very much @lsiepel and @wborn for the thorough review — this was really helpful. Disclosure up front: I run the Live Tennis API, so this binding is vendor-authored; please judge it accordingly. Happy to hand either of you a free key if you'd like to exercise it against the live service. Pushed a round addressing everything below. Per-finding: Doubles — "No support for doubles?" ( Doubles is supported, and I've now made that explicit rather than incidental. The API models a doubles match with the same two-sided shape as singles: To make it visible and usable I added a
429 + timeout → a transient exception that retries ( Don't expose the API client; bridge as proxy ( Listener lifecycle / multiple init ( Wildcard static imports (
thing-types.xml line breaks — Reflowed; README table alignment — Realigned all tables. @wborn — on the One extra, unprompted: Build is green locally ( |
wborn
left a comment
There was a problem hiding this comment.
The latest update addresses a substantial part of the previous feedback. In particular, the API client is now hidden behind the bridge, DTO nullability and doubles handling have improved, child detail requests are gated on an online bridge, and several formatting/documentation issues have been cleaned up.
A few correctness and quota-management issues still need to be addressed:
- The live-match endpoint is paginated, but the first page is still treated as a complete snapshot. The existing pagination thread remains valid.
- The tournament transient-error retry still cannot actually schedule itself because
infoJobis the currently running job when the retry is requested. - Break-point detection assumes that
40-40is never a break point. The REST API does not expose or document the scoring model, so that cannot be determined reliably for matches using no-ad scoring. - A successful live-match response is currently discarded when the subsequent
/usagerequest fails. The existing Copilot review-summary finding remains valid; usage reporting should not prevent publishing otherwise valid live data. - The existing lifecycle concerns around in-flight account/player work publishing state after disposal remain applicable.
- Player detail refreshes can overlap with refreshes triggered when the bridge becomes
ONLINE, causing avoidable quota use around startup/reconnect; this is covered by the existing player lifecycle thread.
The default request intervals should also be reconsidered for the advertised free tier. The bridge alone uses 96 requests/day at the default 900-second interval, and one player adds another 48 requests/day at the default 3600-second detail interval. A basic bridge + player configuration therefore uses 144 requests/day against the 100 requests/day free-tier quota.
There is also a DCO issue with both commits. They currently use Signed-off-by: Ben <118375461+bensynapse@users.noreply.github.com>. openHAB requires the contributor's real name and a reachable, non-GitHub-noreply email address in every sign-off, so the commits need to be rewritten with compliant sign-offs.
For the reviewed revision 1f3c86e, CI is green and the SAT report contains no findings.
This review was AI-assisted.
| return; | ||
| } | ||
| try { | ||
| Player refreshedPlayer = bridge.fetchPlayer(playerId); |
There was a problem hiding this comment.
detailRefreshInterval currently refreshes both the next match and the player profile/ranking, resulting in two counted requests per player every cycle. These data have very different freshness requirements: upcoming-match information benefits from regular polling, while ranking data changes much less frequently.
Could the ranking/profile request be refreshed on a much slower internal schedule (for example daily), while this interval controls only the upcoming-match refresh? That would substantially reduce free-tier quota usage without adding another user-facing configuration option.
- Pagination: MatchListResponse now parses meta; the client pages the live-match list forward on meta.has_more (capped at 5 pages) instead of treating the first page as a complete snapshot, and logs when it deliberately truncates. Falls back to the page-fill heuristic only when meta is absent. - Tournament retry: the transient retry runs on its own retryJob rather than scheduleInfoRefresh, which gated on infoJob — the currently executing job — and so could never schedule. dispose() cancels it. - Break point at 40-40 is now UNDEF: the REST API does not expose the scoring format, so 40-40 is undeterminable (break point under no-ad, deuce under advantage). Only the determinable cases are asserted. - Usage read decoupled from the live poll: live data is published first and a failing /usage call no longer discards it. - Disposal: handlers carry a disposed flag and no longer publish state or status after dispose(). - Player detail overlap: an in-progress guard stops the periodic, ONLINE-triggered and retry refreshes from running concurrently and wasting quota; the tournament fetch gets the same guard. - Defaults raised so a bridge plus one player fit the free tier: bridge 1800 s (48/day) + player 7200 s (24/day) = 72/day; README math fixed. - New player detailRefreshEnabled switch to disable the ranking and next-match refresh entirely, lowering 429 risk. - Tests added for the 40-40 UNDEF behaviour and for the paging loop. Signed-off-by: Ben Abulafia <ben@synapsereality.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the second careful pass, @wborn — you were right on every point. Pushed 610bb01 addressing all of them (I maintain the Live Tennis API, so this is vendor-authored; judge accordingly). 1. Pagination — first page treated as a complete snapshot. 2. Tournament retry could never schedule. 3. Break point at 40-40. 4. 5. Publishing after dispose(). 6. Detail-refresh overlap at startup/reconnect. 7. Default intervals exceeded the free tier. 8. (@lsiepel) Make the detail/stats refresh optional. Tests added for the 40-40 UNDEF behaviour and the paging loop; On the DCO sign-off: acknowledged — the real-name / non-noreply-email correction and history rewrite are being handled separately and will be applied before merge. This commit keeps the sign-off identity currently used on the branch so it doesn't block the review. Happy to hand over a free API key if it helps you exercise the live paths against real data. |
610bb01 to
f3e3a89
Compare
|
DCO sign-off corrected: all three commits are rewritten to |
Since this PR is AI coded and apparently not yet tested, I added the "additional testing preferred" to indicate this. |
There was a problem hiding this comment.
The latest update addresses most of the previous review findings. In particular, normal live-match pagination is now implemented, the tournament retry is functional, 40-40 correctly maps to UNDEF, usage failures no longer discard live data, the defaults fit the free-tier budget, and the commits now have compliant DCO sign-offs.
A few issues still need attention:
- The lifecycle protection is not sufficient across
dispose()→initialize(). An old in-flight request can become valid again afterinitialize()resets the disposed state and can publish data or status belonging to the previous configuration. - The player refresh guard prevents concurrent requests, but not near-sequential duplicate detail refreshes around startup/reconnect or a pending retry.
- The new Java sources use
Ben Synapsein@author, while the commits identify the contributor as Ben Abulafia and the repository guidelines require the real contributor name. - The five-page pagination limit still returns a truncated result as an authoritative snapshot when
meta.has_moreremains true. If this defensive limit is intentional, an incomplete snapshot should not be treated as complete and the README should not claim that pagination continues until the snapshot is complete.
The PR description should also be updated because it still documents the old 900-second bridge default.
For revision f3e3a89, CI is green and the SAT report contains no findings.
This review was AI-assisted.
- Guard dispose()/initialize() lifecycle races with a generation counter in all three handlers: an in-flight request captures the lifecycle value at its start and only publishes data/status while it still matches, so a request already in flight cannot publish state for a disposed or reconfigured handler even after a later initialize() clears the disposed flag. - Add a near-sequential duplicate-refresh guard to the player detail refresh: refreshes that fire within a 30 s spacing window of the previous one are collapsed, so the startup/reconnect burst (initial-delay job, bridge-ONLINE refresh, pending retry) no longer spends double quota. The periodic job and the 60 s transient retry are unaffected. - Stop treating a five-page-capped live snapshot as complete: the README and the client javadoc now state the snapshot is deliberately truncated (and logged as a warning) when meta.has_more is still true at the cap, instead of claiming pagination always continues until complete. - Rename the @author tag from "Ben Synapse" to "Ben Abulafia" across all binding sources to match the contributor's real name. Signed-off-by: Ben Abulafia <hello@livetennisapi.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @wborn and @jlaur — pushed a revision (79460b5) addressing the review:
Full single-binding build is green locally: |
wborn
left a comment
There was a problem hiding this comment.
Thanks for the update. The lifecycle generation checks address stale publication, the 30-second spacing guard addresses the near-sequential player refresh, and the PR description now reflects the 1800-second default.
The current head still needs changes. The latest commit accidentally replaced the README and API client with edit instructions, and CI is red before the binding reaches compilation. The existing pagination-completeness and author-attribution threads also remain unresolved, and the player lifecycle guard can suppress the new lifecycle's initial detail refresh during reconfiguration as described inline.
This review was AI-assisted.
| @@ -0,0 +1,23 @@ | |||
| Two edits to this file (both applied and built): | |||
There was a problem hiding this comment.
These are edit instructions, not Java source. The previous client implementation has been replaced almost entirely, so the binding cannot compile once the earlier README markdownlint failure is fixed. README.md was accidentally replaced in the same way. Please restore both files from the previous revision and apply only the intended localized edits.
Description
This PR contributes a new binding for the Live Tennis API — real-time tennis scores covering ATP, WTA, Challenger, ITF and junior Grand Slam draws.
Vendor disclosure: I run the Live Tennis API, so this is a vendor-authored contribution — judge accordingly. The binding uses only endpoints included in the API's free tier (self-serve key, no card), and the README documents the free tier's request budget honestly. Happy to provide maintainers a free key for review testing.
Classification: Novel Addition (new binding).
What it provides:
accountbridge — holds the API key, polls the live-match snapshot once per refresh interval (default 1800 s, chosen so a bridge plus one player thing fits the free tier's 100 requests/day) and shares that single request with all child things. Exposes the key's own usage channels (tier,calls-today,remaining-today); the usage read is quota-exempt on the API side.playerthing — live match state for a tracked player: score line, sets, in-game points, a serving switch, a derived break-point flag and a tiebreak flag; plus next-match channels (opponent, start time, tournament, round) and the player's current ranking. Its own next-match/ranking refresh runs on a slower detail interval (default 7200 s) and can be switched off entirely.tournamentthing — catalogue metadata (name, surface, category), the number of the tournament's matches in progress and the state of its first listed live match.Design notes:
openweathermapbridge/things shape and recent handler idioms (peblar).UNDEFrather than guessing.meta.has_moreup to a defensive five-page cap. If the API still reports more matches at the cap the snapshot for that cycle is deliberately truncated and logged as a warning, rather than presented as complete.create_openhab_binding_skeleton.sh, which also updatedCODEOWNERS, the bom and the bundles pom.Testing
mvn clean install -pl :org.openhab.binding.livetennisapipasses locally on Java 21, including spotless and the SAT checks (checkstyle/PMD/SpotBugs) with no findings.The work is signed off per the DCO.