Rewrite core framework to use request-response abstraction

[ObsidianMinor]

Part 1 of ??.

This focuses on rewriting the core of the library (the framework module) to use a request/response abstraction. This model is much more open to extension and doesn't constrain users to using a specific client abstraction, allowing them to develop their own as they see fit. As a whole, the API has simplified, hopefully making using and building off the library easier.

However, this means the mocking features have been removed since there's no more clients to mock. Users are expected to make their own mock implementation how they see fit based on the client abstraction they design.

Also part of this change, the implementation is extended to allow for new request and response types, which opens up the library to all existing endpoints in the API (including the Workers APIs).

In the interest of making this PR series easier to review, I'm not deleting the whole thing and starting new on this branch. Instead I'll replace pieces as I go, making it easier to compare changes as we move through this rewrite, with the effect of making all checks fail.

[ObsidianMinor]

I've never seen this used. Do APIs exist that have error fields other than code and message? Do APIs exist that actually have more than one error??? Either way it doesn't seem like a good way to design this sort of API. If an API has a specific set of data it returns as part of its errors, the Endpoint should design a JSON response around that fact.

[adamchalmers]

Unfortunately yes, some APIs do set nonstandard error fields. Last time I checked, the LB team had some rich domain-specific errors here, which is why I included it :(

[ObsidianMinor]

Well luckily with this design we won't need it like this, endpoints can specify their own json error structures.

[ObsidianMinor]

I'm actually not sure about having this as a separate file. The original idea was to reduce dependencies by making it optional (and removing the url dependency with it) but it doesn't really seem that worth it now. It's only 2 structs and an extension trait.

[adamchalmers]

Agree, it doesn't seem worth its own feature. If this let you avoid pulling in another dependency from crates.io, I'd say, sure, go ahead, hide it behind a feature. But if not, let's not worry?

[adamchalmers]

Looks good so far. I think the main thing I'd like to see is examples of what the new request API looks like. The thing I care most about here is the UX, so an example showing how users will use this would be very helpful for me :)

[adamchalmers]

Agree, it doesn't seem worth its own feature. If this let you avoid pulling in another dependency from crates.io, I'd say, sure, go ahead, hide it behind a feature. But if not, let's not worry?

[adamchalmers]

weird to see API capitalized but URL not, we should choose one policy and be consistent

[ObsidianMinor]

I think I made it not capitalized because it refers to the Url type. Should make that obvious with a intra-doc link.

[ObsidianMinor]

There's some examples I converted from the previous crates to the new design that are included in the reqwest integration crate.

    let builder = RequestBuilder::new(credentials);

    let client = Client::new();

    use cloudflare::api::load_balancer;

    let response = load_balancer::pools::Create {
        account_id: &account_id,
        params: load_balancer::pools::CreateParams {
            name: "test-pool".to_owned(),
            description: Some("test description".to_owned()),
            enabled: Some(true),
            minimum_origins: Some(2),
            monitor: Some("9004c07f1c0f33255410e45590251cf4".to_owned()),
            notification_email: Some("[email protected]".to_owned()),
            origins: vec![
                load_balancer::pools::Origin {
                    name: "test-origin".to_owned(),
                    address: Ipv4Addr::new(152, 122, 3, 1).into(),
                    enabled: true,
                    weight: 1.0,
                },
                load_balancer::pools::Origin {
                    name: "test-origin-2".to_owned(),
                    address: Ipv4Addr::new(152, 122, 3, 2).into(),
                    enabled: true,
                    weight: 1.0,
                },
            ],
        },
    }
    .request(&builder)
    .send(&client)
    .await
    .context("Failed to create pool")?;
    let pool = match response {
        JsonResponse {
            body:
                JsonResult {
                    success: true,
                    result: Some(pool),
                    ..
                },
            ..
        } => pool,
        response => anyhow::bail!("Failed to create pool: {:#?}", response),
    };

Definitely open to making the match (which is used often) into a converter that returns a result.

    .request(&builder)
    .send(&client)
    .await
    .context("Failed to create pool")?
    .success_ok()
    .map_err(|r| anyhow!("API call failed: {:#?}", r))?;

[adamchalmers]

nit: I'd prefer if the shorter name "build" was the safe one (i.e. doesn't panic), and the panicking method had a longer name than "build". Or maybe we could remove the panic, and just let users call .build().unwrap() if they really want to force the panic. What do you think?

(same idea applies to the request/try_request extension methods below)

[ObsidianMinor]

I prefer shorter names to be obvious happy paths that most users will take. 99.99% of the time the user will have a URL that supports joining with paths (if they use the default URL this will be true) and endpoints will have paths that can be joined with a standard base URL (if they use included endpoints, this will also be true), anything else is something a user probably doesn't want to handle, but if they do, they can use the try_ methods.

[Noah-Kennedy]

I think this is fine as is.

[Noah-Kennedy]

I'm actually going to remove my approval for now. After talking with @sssilver, I think that we should probably have this specced out first and discuss that, then we should do the change atomically in one PR or in a series of semi-atomic PRs that do not at any point leave the repo in a state where CI is broken.