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

Add Search Paramater Re-Insertion to Child Tile URLs in RealityDataSourceTilesetUrlImpl #7352

Merged
merged 9 commits into from
Nov 13, 2024

Conversation

andremig-bentley
Copy link
Contributor

This PR adds _searchParams to the RealityDataSourceTilesetUrlImpl class in RealityDataSourceTilesetUrlImpl.ts. This paramater is an empty string until setBaseUrl is called, at which point, it will be set to the search parameters of the associated url if they exist. The search parameters will then be reinserted into the tile url in getTileUrl.

This is a necessary step when attaching reality data using a url from blob storage, an example being attaching a Cesium export of an iModel from the Mesh Export Service. Without these changes, using attachRealityModel() with the url provided by the MES would not work. Requests for child tiles would be unauthorized because the url would not include the search parameters/sas token from the original tileset url.

We did not think any sort of toggle or flag was necessary and that this behavior could be applied to all urls because we believe that any instance of a url containing search parameters would be an instance that needed those parameters passed on, and in all other instances, _searchParams would remain as an empty string.

@eringram @markschlosseratbentley

@andremig-bentley andremig-bentley requested a review from a team as a code owner November 12, 2024 22:28
@eringram
Copy link
Member

Looks good to me but I'd recommend waiting to merge for another approval too.

Also a note on what the 3D Tiles spec says about this, I found this discussion in the past where apparently query parameters shouldn't be inherited by child tiles according to the specification: CesiumGS/3d-tiles#746. However, in practice, they are inherited when CesiumJS reads 3D Tiles tilesets, and also Google Photorealistic Tiles uses some query params on the root tile similar to the MES

@danieliborra danieliborra merged commit a5b50bb into master Nov 13, 2024
16 checks passed
@danieliborra danieliborra deleted the andremig/realitydata-url branch November 13, 2024 19:11
@andremig-bentley
Copy link
Contributor Author

@aruniverse @markschlosseratbentley @ben-polinsky Can this PR be backported? It's currently blocking integration of the Repositories API into iTwin viewers, and as of discussions with @danieliborra, it's incredibly important to get it through as quickly as possible.

@aruniverse
Copy link
Member

@Mergifyio backport release/4.10.x release/4.11.x

Copy link
Contributor

mergify bot commented Nov 13, 2024

backport release/4.10.x release/4.11.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 13, 2024
…urceTilesetUrlImpl (#7352)

Co-authored-by: andremig-bentley <andremig-bentley@users.noreply.github.com>
Co-authored-by: Erin Ingram <47707444+eringram@users.noreply.github.com>
(cherry picked from commit a5b50bb)
mergify bot pushed a commit that referenced this pull request Nov 13, 2024
…urceTilesetUrlImpl (#7352)

Co-authored-by: andremig-bentley <andremig-bentley@users.noreply.github.com>
Co-authored-by: Erin Ingram <47707444+eringram@users.noreply.github.com>
(cherry picked from commit a5b50bb)
aruniverse pushed a commit that referenced this pull request Nov 13, 2024
…urceTilesetUrlImpl (backport #7352) [release/4.11.x] (#7365)

Co-authored-by: andremig-bentley <101671244+andremig-bentley@users.noreply.github.com>
aruniverse pushed a commit that referenced this pull request Nov 13, 2024
…urceTilesetUrlImpl (backport #7352) [release/4.10.x] (#7364)

Co-authored-by: andremig-bentley <101671244+andremig-bentley@users.noreply.github.com>
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.

5 participants