Return parent tile key for aggregatedData request - #544
Merged
Merged
Conversation
For some maps, it's essential to know the actual tile key of the aggregated data as that's unfortunately not part of the payload. Also refactor the logic for aggregated data - it was mostly wrong. Start iterating the subQuadKeys until we have a hit, otherwise take the closest parent. That should guarantee the closest ancestor even if it's more than 4 levels up the tree. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
Cover VersionedLayerClient.getAggregatedData()'s new includeTileKey
overload: the default call keeps returning a plain Response, and
passing { includeTileKey: true } returns the TileResponse with the
resolved parentTileKey.
Also add a regression test for requesting an aggregated tile at a
level not deeper than the quad tree index depth (4). In that case the
quad tree index root is clamped to the global root tile, and
findClosestParent() currently throws "Cannot get the parent of the
root tile key" while walking up the sub quad chain, instead of falling
back to parentQuads. This test currently fails; it is fixed in the
next commit.
Relates-To: DATASDK-108
Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
When the requested tile's level is not deeper than the quad tree index depth (4), TileKey.changedLevelBy() clamps quadTreeIndexRoot to the global root tile (level 0). findClosestParent() then walks the sub quad chain up to and including that root, but unconditionally called currentTileKey.parent() afterwards even once currentTileKey was already the root - and TileKey.parent() throws on the root instead of returning a tile. The exception pre-empted the parentQuads fallback that this same commit's logic depends on to guarantee the closest ancestor. Stop the walk once currentTileKey reaches quadTreeIndexRoot instead of requesting its parent, so the lookup falls through to parentQuads as intended. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
getTile() now returns TileResponse ({ response, parentTileKey })
instead of a plain Response. VersionedLayerClient.ts,
VolatileLayerClient.ts and the unit test already accounted for this,
but this integration test calls getTile() directly and still called
.blob() on the result, so it only failed once a build actually
reflected the new getTile.ts (this repo's root-level integration and
api-breaks suites resolve @here/olp-sdk-dataservice-read via its
compiled output, not the TypeScript source, so a stale build made the
break invisible locally until a fresh npm run build surfaced it).
Read the blob through the new .response field instead.
Relates-To: DATASDK-108
Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
getTile() is re-exported from the package root, so any consumer could
already be calling it directly and relying on the plain Response it
used to return. Changing its return type to TileResponse, as this
branch previously did, silently breaks that caller on the next patch
release: no test in this repo guards getTile()'s public shape (there
is no api-breaks test for it), which is how this slipped through.
Give the free function the same two-overload treatment already used
on VersionedLayerClient.getAggregatedData(): the three-argument call
keeps returning Promise<Response> unchanged, and a new fourth
argument, { includeTileKey: true }, opts in to Promise<TileResponse>.
The shared logic moves into an unexported fetchTile() that always
resolves the full TileResponse; both overloads and both Client
classes are built on top of it.
VersionedLayerClient.getAggregatedData() now asks fetchTile() for the
full TileResponse via that fourth argument, since it needs
parentTileKey to honor its own includeTileKey option.
VolatileLayerClient.getAggregatedData() goes back to forwarding its
arguments straight to getTile(), since it never needed the tile key.
The two pre-existing getTile() tests ("204 response" and "blob of the
requested tile") go back to their original, unmodified form, since
the three-argument call is unchanged again. Their includeTileKey
coverage moves into new, separate tests instead of being folded into
them.
Relates-To: DATASDK-108
Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
getTile() is exported from the package root, but no test in tests/integration/api-breaks/ covered it, which is how its return type silently changed from Response to TileResponse earlier on this branch without any test catching it. Guard the three-argument call (a plain Response, including that an AbortSignal is still accepted as the third argument) and the new includeTileKey opt-in (a TileResponse) against a future incompatible change. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
OleksiiZubko
force-pushed
the
DATASDK-108
branch
from
September 21, 2026 15:04
320c6ef to
da7374e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #544 +/- ##
================================================
+ Coverage 90.9958% 91.7196% +0.7238%
================================================
Files 86 86
Lines 2832 2838 +6
Branches 390 398 +8
================================================
+ Hits 2577 2603 +26
+ Misses 108 90 -18
+ Partials 147 145 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
I don't understand how the tests passed? Anyway, this fix should make CI pass I hope |
Codecov's patch coverage check flagged fetchTile() and findClosestParent() in getTile.ts: 1 fully missing line and 5 partial branches, all inside code this branch added or rewrote. - The cached-quad-tree-index path (request.getFetchOption() !== FetchOptions.OnlineOnly) had no test at all, so cache.get() never ran. - The `?? []` fallbacks for an omitted subQuads/parentQuads field, in both fetchTile() and findClosestParent(), were only ever exercised with an explicit empty array, never with the field missing. - findClosestParent()'s parentQuads reduce always saw candidates in increasing depth order, so the branch that keeps the current closest match instead of replacing it never ran. Add one test per gap: a cache hit that skips QueryApi.quadTreeIndex, a missing subQuads field, a missing parentQuads field, and a parentQuads list with the deepest match first. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
This branch adds the aggregated-tile-key feature and its follow-up fixes (crash fix, non-breaking overload, coverage), so the package needs a patch release once it lands. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
The file was added this year, so it does not need the 2020 start year copied from older test files in the same directory. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
fetchQuadTreeIndex() gated its cache.put() on a truthy subQuads field, which predates the parentQuads fallback this branch added. A response that carries only parentQuads - exactly the case that fallback exists for - was therefore never cached, so every getAggregatedData() call for that root re-fetched the index over the network. Gate on either field being present instead, and add a regression test that fails without the fix. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
The api-breaks suite guards the exported surface with subclasses such as VersionedLayerClientTest, which only fail as compile errors. Vitest transpiles without type checking, and the CI script never invoked the suite at all, so those guards never ran anywhere. Add getAggregatedData to the guard, since it is the method this branch changed, run tsc over tests/ before the suite, and call the suite from the CI script. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
The includeTileKey overload typed its options object as
{ includeTileKey: true }, so TypeScript callers could pass the
literal true and nothing else: { includeTileKey: false } and a
boolean held in a variable were both compile errors, even though
the implementation and the JSDoc describe it as a flag.
Add two overloads to getTile() and to
VersionedLayerClient.getAggregatedData(): one for an omitted or
literal false flag, resolving a plain Response, and one for a
run-time boolean, resolving the union that the caller narrows.
The union overload comes last so the literal forms keep their
precise return types.
Relates-To: DATASDK-108
Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
The earlier bump on this branch went to 3.0.1, but the branch adds exported API - TileResponse, the getTile() and getAggregatedData() overloads - and changes runtime behavior, so a patch release understates it. Consumers on a pinned-patch policy would pick the behavior change up as a safe update. Go to 3.1.0 instead and add the missing CHANGELOG entry, spelling out both behavior changes: parentQuads data now replaces the 204 that an empty sub tree used to produce, for volatile layers too, and the rejection message now carries a HERE tile key. Relates-To: DATASDK-108 Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
Patch update of the test runner and its V8 coverage provider. Relates-To: MINOR Signed-off-by: Oleksii Zubko <ext-oleksii.zubko@here.com>
ystefinko
approved these changes
Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For some maps, it's essential to know the actual tile key of the aggregated data as that's unfortunately not part of the payload.
Also refactor the logic for aggregated data - it was mostly wrong. Start iterating the subQuadKeys until we have a hit otherwise, take the closest parent. That should guarantee the closest ancestor even if it's more than 4 levels up the tree.