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

Replace url::Url with our own type #2053

Open
heaths opened this issue Feb 3, 2025 · 2 comments
Open

Replace url::Url with our own type #2053

heaths opened this issue Feb 3, 2025 · 2 comments
Labels
Azure.Core The azure_core crate Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@heaths
Copy link
Member

heaths commented Feb 3, 2025

Before we GA, I would like to replace the url::Url type that we re-export from typespec_client_core and azure_core with our own type that is better optimized for scenarios we need e.g.,

  1. Mutable query parameters. Our generated client code mutates URLs enough that taking an owned iterator just to reserialize the entire URL later becomes expensive over time.
  2. Ability to construct from a &str for which we are reasonably sure it's a URL e.g., returned resource IDs. This could be exposed via From<&str> or a new_unchecked constructor.
  3. Considering the ability to construct from a &str parts could be retrieved as Cow<'_, str> and, thus, we'd implement ToOwned and friends.

The scenarios all this enables are:

  1. Ability to mutate parameters more with less CPU time and likely no significant memory overhead (we should measure speed and allocations; see https://github.com/heaths/ordinal-rs/pull/13/files for one possibility).
  2. Ability to return cheap &str slices for parts within a URL.
@heaths heaths added Azure.Core The azure_core crate Client This issue points to a problem in the data-plane of the library. labels Feb 3, 2025
@heaths heaths added this to the 2025-03 milestone Feb 3, 2025
@github-project-automation github-project-automation bot moved this to Untriaged in Azure SDK Rust Feb 3, 2025
@heaths
Copy link
Member Author

heaths commented Feb 3, 2025

Initially, I imagine some structure like this (not necessarily sound - just some high-level ideating):

#[derive(Default)] // ...
pub struct Url<'a>(private::Url<'a>);

impl<'a> Url<'a> {
  pub fn new() -> Self {
    Default::default()
  }
  pub fn new_unchecked(url: &'a str) -> Self {
    Self(private::Url::Owned(url))
  }
}

// impl From, FromStr, ToOwned, AsRef, etc.

mod private {
  #[derive(Debug, Default)] // ...
  pub enum Url<'a> {
    Borrowed(&'a str),
    #[default]
    Owned {
      value: String,
      params: HashMap<String, Cow<'a, str>>,
      fragment: Option<String>,
    }
  }
}

/cc @analogrelay @LarryOsterman

@RickWinter RickWinter moved this from Untriaged to Not Started in Azure SDK Rust Feb 5, 2025
@heaths
Copy link
Member Author

heaths commented Feb 27, 2025

Alternatively, we newtype url::Url for now and expose only limited methods we think customers would use normally. If they want more from url::Url, we'll support impl From<url::Url> for Url and impl From<Url> for url::Url and they can get out of what they need. But this means if we do ever replace the 3P dependency completely1 we'd still have to support that conversion.

Footnotes

  1. Assuming that we even can remove it. Right now, cargo tree -i url shows it's only used by our crates, http-types - which we want to remove in Replace http-types with http #2233 - and fe2o3-amqp which is only used for azure_core_amqp-based crates, which will only be a couple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core The azure_core crate Client This issue points to a problem in the data-plane of the library.
Projects
Status: Not Started
Development

No branches or pull requests

1 participant