Skip to content
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
efead0a
Parts of implementation using ada-url instead of uriparser
azrogers Jan 15, 2025
b397b06
Merge branch 'main' of github.com:CesiumGS/cesium-native into ada-url
azrogers Jan 16, 2025
9617622
Part of a Uri refactor
azrogers Jan 17, 2025
ad542c1
It's working!?
azrogers Jan 17, 2025
6fe2fdf
Document methods
azrogers Jan 17, 2025
f57f7d9
Remove cruft
azrogers Jan 17, 2025
45d9d7f
Merge from main
azrogers Jan 17, 2025
6115d11
Format
azrogers Jan 17, 2025
eadf419
Fix clang-tidy issues
azrogers Jan 17, 2025
887f981
Fix Doxygen issues
azrogers Jan 17, 2025
68aebad
Remove find_package(uriparser)
kring Jan 19, 2025
78f0f40
Re-add uriparser for now, and fix macOS compile error.
kring Jan 19, 2025
e777426
Fix CI issues
azrogers Jan 21, 2025
38c1e31
Format
azrogers Jan 21, 2025
ea46377
Rewrite substituteTemplateParameters to work with encoded brackets
azrogers Jan 23, 2025
605ea5a
Format
azrogers Jan 23, 2025
40d4ba9
Merge branch 'main' of github.com:CesiumGS/cesium-native into ada-url
azrogers Jan 23, 2025
84cc7c7
Merge branch 'main' of github.com:CesiumGS/cesium-native into ada-url
azrogers Jan 23, 2025
8c84337
A few more tests for edge cases, regex implementation
azrogers Jan 24, 2025
4b3e558
Merge branch 'main' of github.com:CesiumGS/cesium-native into ada-url
azrogers Jan 24, 2025
369d3a8
Merge remote-tracking branch 'origin/main' into ada-url
kring Jan 28, 2025
17eefa1
Substitute placeholders before manipulating layer.json URLs.
kring Jan 28, 2025
158de7e
Merge branch 'main' of github.com:CesiumGS/cesium-native into ada-url
azrogers Jan 28, 2025
9789911
Address most of review
azrogers Jan 28, 2025
9e032f8
Merge pull request #1084 from CesiumGS/ada-url-substitute-first
azrogers Jan 28, 2025
cb18f76
Return to previous substitution logic
azrogers Jan 28, 2025
7006b81
Remove uriparser, format, etc
azrogers Jan 28, 2025
3d5d39d
Update dependency graphs
azrogers Jan 28, 2025
c3a18c6
Format
azrogers Jan 28, 2025
1c4e4c2
Merge from main
azrogers Jan 29, 2025
db5868a
Address review comments
azrogers Jan 29, 2025
46ad1ac
Separate query handling from Uri
azrogers Jan 29, 2025
cea035d
Fix clang-tidy header warning
azrogers Jan 30, 2025
b0a16d2
Fix docs
azrogers Jan 30, 2025
f34c8fe
Format
azrogers Jan 30, 2025
eb016f0
Rename UriQueryParams -> UriQuery
azrogers Jan 31, 2025
9263109
Merge remote-tracking branch 'origin/main' into ada-url
kring Feb 1, 2025
8d319e4
Correct comment that is no longer true.
kring Feb 1, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ message(STATUS "VCPKG_OVERLAY_TRIPLETS ${VCPKG_OVERLAY_TRIPLETS}")
# These packages are used in the public headers of Cesium libraries, so we need to distribute the headers and binaries
# with the installation
# Note that fmt is a public dependency of the vcpkg version of spdlog
# STB and uriparser aren't technically part of the public interface, but they're used by the downstream Cesium for Unreal project
set(PACKAGES_PUBLIC asyncplusplus expected-lite fmt glm rapidjson spdlog stb uriparser)
# STB and ada-url aren't technically part of the public interface, but they're used by the downstream Cesium for Unreal project
set(PACKAGES_PUBLIC asyncplusplus expected-lite fmt glm rapidjson spdlog stb ada-url uriparser)
# These packages are used in the code and produce binaries, but are not part of the public interface. Therefore we need
# to distribute the binaries for linking, but not the headers, as downstream consumers don't need them
# OpenSSL and abseil are both dependencies of s2geometry
Expand Down Expand Up @@ -257,6 +257,7 @@ find_package(tinyxml2 CONFIG REQUIRED)
find_package(unofficial-sqlite3 CONFIG REQUIRED)
find_package(uriparser CONFIG REQUIRED char wchar_t)
find_package(WebP CONFIG REQUIRED)
find_package(ada CONFIG REQUIRED)


# Private Library (s2geometry)
Expand Down
13 changes: 8 additions & 5 deletions Cesium3DTilesSelection/test/TestLayerJsonTerrainLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,8 @@ TEST_CASE("Test create layer json terrain loader") {
CHECK(layers[0].version == "1.33.0");
CHECK(
layers[0].tileTemplateUrls.front() ==
"{z}/{x}/{y}.terrain?v={version}&extensions=octvertexnormals-metadata");
"%7Bz%7D/%7Bx%7D/"
"%7By%7D.terrain?v=%7Bversion%7D&extensions=octvertexnormals-metadata");
Copy link
Member

@kring kring Jan 20, 2025

Choose a reason for hiding this comment

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

This makes me a little nervous. I guess it's needed because { and } aren't valid characters in a URL path, right? But it gets tricky fast. What if the URL already contains %7B and %7D characters? How do we distinguish the ones that are really the placeholder delimiters from the others that might be in the URL?

The best solution, I think, is for the URL library to understand placeholders. That sounds like a tall order, but it looks like ada added support two weeks ago!
ada-url/ada#785

This isn't in a released version. And it might not fit perfectly; we might need to replace {x} with {:x}, if I'm reading it correctly. Still, this should be a lot more reliable than trying to encode/decode URLs with placeholders.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would probably be good to switch to the URLPattern implementation. It looks like it would speed up placeholder substitution (not that it's a big performance cost for us right now, but a win is a win), and having our placeholders in line with the spec can't hurt. My question is, is there anywhere in the codebase currently where users can input their own templated URLs? Besides UrlTemplateRasterOverlay, of course, which hasn't yet been shipped so it's OK if we change the behavior. I'd hate for us to update and break everyone's projects because they don't have colons in their URL templates.

As for it not being in a released version, that might be a concern. It doesn't look like there's any defined release cadence for ada, since the last release was in September. Using this might mean we have to add it as a submodule - which wouldn't be the end of the world, but I would prefer not to if we can avoid it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do wonder how many URLs contain encoded { and } characters that aren't URL templates that we should be worried about.

Copy link
Member

Choose a reason for hiding this comment

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

I do wonder how many URLs contain encoded { and } characters that aren't URL templates that we should be worried about.

Well, it's not just the { and } we have to worry about, though. It looks like URLs are now percent-decoded before doing the placeholder substitution. So if you had, say, a percent encoded slash, that would get decoded, too, and could break the URL. How common is this? I don't know, but it definitely came up in CesiumJS at one point. CesiumJS replaces just the { and } characters:
https://github.com/CesiumGS/cesium/blob/main/packages/engine/Source/Core/Resource.js#L550

Using this might mean we have to add it as a submodule - which wouldn't be the end of the world, but I would prefer not to if we can avoid it.

No need for a submodule, it can be done with a vcpkg overlay port. We haven't used those in cesium-native itself yet, though (only Unreal and Unity).

is there anywhere in the codebase currently where users can input their own templated URLs

Yeah, the WMTS tile provider uses placeholders.

I suspect we could simply replace {whatever} with {:whatever} on-the-fly, though, and that would be reliable.

Copy link
Contributor Author

@azrogers azrogers Jan 23, 2025

Choose a reason for hiding this comment

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

@kring After looking into ada's URL pattern support, I think it would be better if we avoided it. Besides the URL pattern support being fairly sparse on documentation, not yet in a stable release, and not currently building on GCC versions lower than 13.1, it's actually meant to do the opposite of what we're looking to do. It's meant to match a pattern against a URL, such as for the purposes of routing (like how Express.js uses them), rather than using a pattern to generate a URL from a set of parameters. We could probably finagle the library to work this way, but it would be a bit of a headache and doesn't seem too worth it all things considered. Probably would not be particularly more performant than our current implementation.

Instead, I've rewritten substituteTemplateParameters so that encoded { and } characters are treated as if they are their unencoded equivalents. This allows the templates to be substituted without decoding the entire string, saving us a string copy as well as preventing any issues from URL decoding things we shouldn't be. Technically, this does still have the issue where an encoded { } that isn't meant to be treated as a placeholder will still be treated as one, but considering CesiumJS would have the same issue I think it's a fine solution.

CHECK(layers[0].loadedSubtrees.size() == 2);
CHECK(layers[0].availabilityLevels == 10);
}
Expand Down Expand Up @@ -290,7 +291,8 @@ TEST_CASE("Test create layer json terrain loader") {
CHECK(layers[0].version == "1.0.0");
CHECK(
layers[0].tileTemplateUrls.front() ==
"{z}/{x}/{y}.terrain?v={version}&extensions=octvertexnormals");
"%7Bz%7D/%7Bx%7D/"
"%7By%7D.terrain?v=%7Bversion%7D&extensions=octvertexnormals");
CHECK(layers[0].loadedSubtrees.empty());
CHECK(layers[0].availabilityLevels == -1);

Expand All @@ -313,7 +315,7 @@ TEST_CASE("Test create layer json terrain loader") {
auto parentJsonPath =
testDataPath / "CesiumTerrainTileJson" / "Parent.tile.json";
pMockedAssetAccessor->mockCompletedRequests.insert(
{"./Parent/layer.json", createMockAssetRequest(parentJsonPath)});
{"Parent/layer.json", createMockAssetRequest(parentJsonPath)});

auto loaderFuture =
LayerJsonTerrainLoader::createLoader(externals, {}, "layer.json", {});
Expand Down Expand Up @@ -411,8 +413,9 @@ TEST_CASE("Test create layer json terrain loader") {
CHECK(layers[0].tileTemplateUrls.size() == 1);
CHECK(
layers[0].tileTemplateUrls[0] ==
"{z}/{x}/"
"{y}.terrain?v={version}&extensions=octvertexnormals-watermask");
"%7Bz%7D/%7Bx%7D/"
"%7By%7D.terrain?v=%7Bversion%7D&extensions=octvertexnormals-"
"watermask");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"geometricError": 0,
"refine": "ADD",
"content": {
"uri": "tileset3/ll.b3dm"
"uri": "ll.b3dm"
}
}
}
2 changes: 1 addition & 1 deletion CesiumUtility/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,5 @@ target_link_libraries(
zlib-ng::zlib-ng
spdlog::spdlog
glm::glm
uriparser::uriparser
ada::ada
)
117 changes: 113 additions & 4 deletions CesiumUtility/include/CesiumUtility/Uri.h
Original file line number Diff line number Diff line change
@@ -1,15 +1,107 @@
#pragma once

#include <ada.h>

#include <functional>
#include <memory>
#include <optional>
#include <string>
#include <string_view>

namespace CesiumUtility {

/**
* @brief A class for building and manipulating Uniform Resource Identifiers
* @brief A class for parsing and manipulating Uniform Resource Identifiers
* (URIs).
*
* The URI parser supports the [WhatWG URL
* specification](https://url.spec.whatwg.org/). It also supports
* protocol-relative URIs such as `//example.com`, and opaque paths such as
* `a/b/c`.
*/
class Uri final {
public:
/**
* @brief Attempts to create a new Uri by parsing the given string. If the
* string fails to parse, \ref isValid will return false.
*
* @param uri A string containing the URI to attempt to parse.
*/
Uri(const std::string& uri);
/**
* @brief Attempts to create a new Uri from a base URI and a relative URI. If
* the base URI is invalid or the relative URI string fails to parse, \ref
* isValid will return false.
*
* @param base The base URI that the relative URI is relative to.
* @param relative A string containing a relative URI to attempt to parse.
* @param useBaseQuery If true, the resulting URI will include the query
* parameters of both the base URI and the relative URI. If false, only the
* relative URI's query parameters will be used (if any).
*/
Uri(const Uri& base, const std::string& relative, bool useBaseQuery = false);

/**
* @brief Copy constructor for Uri.
*/
Uri(const Uri& uri);

/**
* @brief Returns a string representation of the entire URI including path and
* query parameters.
*/
std::string toString() const;

/**
* @brief Returns true if this URI has been successfully parsed.
*/
bool isValid() const;

/**
* @brief Equivalent to \ref isValid.
*/
operator bool() const { return this->isValid(); }

/**
* @brief Obtains the value of the given key from the query string of the URI,
* if possible.
*
* If the URI can't be parsed, or the key doesn't exist in the
* query string, `std::nullopt` will be returned.
*
* @param key The key whose value will be obtained from the URI, if possible.
* @returns The value of the given key in the query string, or `std::nullopt`
* if not found.
*/
const std::optional<std::string_view>
getQueryValue(const std::string& key) const;

/**
* @brief Sets the given key in the URI's query parameters to the given value.
* If the key doesn't exist already, it will be added to the query parameters.
* Otherwise, the previous value will be overwritten.
*
* @param key The key to be added to the query string.
* @param value The value to be added to the query string.
*/
void setQueryValue(const std::string& key, const std::string& value);

/**
* @brief Gets the path portion of the URI. This will not include query
* parameters, if present.
*
* @return The path, or empty string if the URI could not be parsed.
*/
const std::string_view getPath() const;

/**
* @brief Sets the path portion of a URI to a new value. The other portions of
* the URI are left unmodified, including any query parameters.
*
* @param path The new path portion of the URI.
*/
void setPath(const std::string_view& path);

/**
* @brief Attempts to resolve a relative URI using a base URI.
*
Expand All @@ -21,10 +113,11 @@ class Uri final {
* @param relative The relative URI to be resolved against the base URI.
* @param useBaseQuery If true, any query parameters of the base URI will be
* retained in the resolved URI.
* @param assumeHttpsDefault If true, protocol-relative URIs (such as
* `//api.cesium.com`) will be assumed to be `https`. If false, they will be
* assumed to be `http`.
* @param assumeHttpsDefault This parameter is ignored and is only kept for
* API compatibility.
* @returns The resolved URI.
*
* @deprecated Use the \ref Uri constructor instead.
*/
static std::string resolve(
const std::string& base,
Expand All @@ -40,6 +133,9 @@ class Uri final {
* @param key The key to be added to the query string.
* @param value The value to be added to the query string.
* @returns The modified URI including the new query string parameter.
*
* @deprecated Create a \ref Uri instance and use \ref Uri::setQueryValue
* instead.
*/
static std::string addQuery(
const std::string& uri,
Expand All @@ -56,6 +152,9 @@ class Uri final {
* @param key The key whose value will be obtained from the URI, if possible.
* @returns The value of the given key in the query string, or an empty string
* if not found.
*
* @deprecated Create a \ref Uri instance andthe member function \ref
* getQueryValue instead.
*/
static std::string
getQueryValue(const std::string& uri, const std::string& key);
Expand Down Expand Up @@ -193,6 +292,8 @@ class Uri final {
*
* @param uri The URI from which to get the path.
* @return The path, or empty string if the URI could not be parsed.
*
* @deprecated Create a \ref Uri instance and use \ref Uri::getPath() instead.
*/
static std::string getPath(const std::string& uri);

Expand All @@ -204,8 +305,16 @@ class Uri final {
* @param newPath The new path portion of the URI.
* @returns The new URI after setting the path. If the original URI cannot be
* parsed, it is returned unmodified.
*
* @deprecated Create a \ref Uri instance and use \ref Uri::setPath(const
* std::string_view&) instead.
*/
static std::string
setPath(const std::string& uri, const std::string& newPath);

private:
std::unique_ptr<ada::url_aggregator> url = nullptr;
std::unique_ptr<ada::url_search_params> params = nullptr;
bool hasScheme = false;
};
} // namespace CesiumUtility
Loading