fix: handle discovery URL correctly#41
Conversation
Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
Handle discovery base urls as url::Url instead of String. Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
|
@thomas-fossati can you have a look at it? |
thomas-fossati
left a comment
thomas-fossati
left a comment
There was a problem hiding this comment.
LGTM, thanks! I have left my usual handful of comments inlnie.
| .verification_url | ||
| .ok_or_else(|| Error::ConfigError("missing API endpoint".to_string()))?; | ||
| let Some(base_url) = &self.base_url else { | ||
| Err(Error::ConfigError("missing API endpoint".to_string()))? |
There was a problem hiding this comment.
not wrong but reads a bit odd :)
| Err(Error::ConfigError("missing API endpoint".to_string()))? | |
| return Err(Error::ConfigError("missing API endpoint".to_string())) |
| let verification_url = base_url.join("/.well-known/veraison/verification").unwrap(); | ||
| let coserv_url = base_url.join("/.well-known/coserv-configuration").unwrap(); |
There was a problem hiding this comment.
Purely stylistic comment: .expect("...") would document intent a bit more explictly.
| ); | ||
|
|
||
| let CoservQuery::EnvQuery(query) = coserv_out.query else { | ||
| panic!("query is not an environemnt query") |
There was a problem hiding this comment.
typo
| panic!("query is not an environemnt query") | |
| panic!("query is not an environment query") |
| .coserv_url | ||
| .ok_or_else(|| Error::ConfigError("missing API endpoint".to_string()))?; | ||
| // since paths are compile time constants, these will always be Ok(_) | ||
| let verification_url = base_url.join("/.well-known/veraison/verification").unwrap(); |
There was a problem hiding this comment.
Using join does reference resolution and therefore changes the previous behaviour (for the better!). Since the join argument starts with /, it's an absolute-path reference which replaces the entire base path.
Previously https://host/veraison + /.well-known/... would yield https://host/veraison/.well-known/...
Now: Url::parse("https://host/veraison").join("/.well-known/...") yields https://host/.well-known/...
This is more correct IMO, but may break some user's assumptions :-)
(Worth documenting.)
4 participants
No description provided.