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

Proposal for new auto discovery results #427

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
130 changes: 129 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion wp_api/src/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::sync::Arc;
use wp_serde_helper::deserialize_i64_or_string;

pub use login_client::WpLoginClient;
pub use url_discovery::{UrlDiscoveryState, UrlDiscoverySuccess};
pub use url_discovery::{UrlDiscoveryError, UrlDiscoveryState, UrlDiscoverySuccess};

use crate::ParsedUrl;
use crate::WpUuid;
Expand Down
29 changes: 29 additions & 0 deletions wp_api/src/login/url_discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,35 @@ use super::WpApiDetails;

const API_ROOT_LINK_HEADER: &str = "https://api.w.org/";

#[uniffi::export]
fn new_api_discovery(site_url: String) -> NewUrlDiscoveryResult {
todo!()
}

#[derive(Debug, uniffi::Record)]
pub struct NewUrlDiscoveryResult {
pub user_input_attempt: NewUrlDiscoveryAttemptResult,
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if this can (or should) be a Result<NewUrlDiscoveryAttemptResult, UrlDiscoveryAttemptError> – that'd avoid the is_successful property (consuming code needs to branch either on a switch statement or on if is_successful so I don't think increases the costs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uniffi doesn't let us use Results in fields. They can only be used as return types for functions. The likely reason being, the errors get mapped to exceptions in other languages - as they don't all have a Result type that's similar to Rust.

pub other_successful_attempts: Vec<NewUrlDiscoveryAttemptResult>,
pub other_failed_attempts: Vec<NewUrlDiscoveryAttemptResult>,
}
Comment on lines +18 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Vec<NewUrlDiscoveryAttemptResult> might be part of the usability trouble.

WDYT about having something like:

Suggested change
pub struct NewUrlDiscoveryResult {
pub user_input_attempt: NewUrlDiscoveryAttemptResult,
pub other_successful_attempts: Vec<NewUrlDiscoveryAttemptResult>,
pub other_failed_attempts: Vec<NewUrlDiscoveryAttemptResult>,
}
pub struct NewUrlDiscoveryResult {
// If the user provides a fully-formed valid URL, prefer that over any
// "smart" things we might try to do
pub user_input_attempt: NewUrlDiscoveryAttemptResult,
// If the user didn't provide a scheme, we'll try HTTP and HTTPs
// If they provide HTTP we'll try HTTPs just in case
pub auto_https_attempt: Option<NewUrlDiscoveryAttemptResult>,
// If the user didn't provide a scheme, we'll try HTTP and HTTPs
pub no_https_attempt: Option<NewUrlDiscoveryAttemptResult>,
// If the autodiscovery process results in a redirect
pub redirected_attempt: Option<NewUrlDiscoveryAttemptResult>,
pub is_successful: bool,
pub is_failure: bool,
// This is a copy of whichever of the above attempts was successful
pub success: NewUrlDiscoveryAttemptResult,
// Return the most detailed error we can – probably just the one
// from the attempt that got the farthest in the process?
pub error: UrlDiscoveryAttemptError
}

IMHO it's rare that you'd want to iterate through the attempts (failed or otherwise) without explaining to the user what you tried, so having explicit names for what we attempted would make the API more discoverable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently don't auto attempt http as it wouldn't be secure. If the user intentionally inputs an http url, then the discovery will work.

We probably wouldn't want both is_successful & is_failure since they are opposite of each other 😅

As I mentioned on Slack, I don't think the error belongs here as we can't make a good guess for which error should be raised. We need the error type to be in individual attempt result instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh also, I don't think we are doing a separate attempt for redirects - but they might already work because the request executor handles it for us. I'd need a test case to double check this.


#[derive(Debug, uniffi::Record)]
pub struct NewUrlDiscoveryAttemptResult {
pub is_network_error: bool,
pub is_successful: bool,
pub is_the_site_url_same_as_the_user_input: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub is_the_site_url_same_as_the_user_input: bool,
pub is_the_site_url_same_as_the_user_input: bool,
// Is this site running WordPress?
pub is_wordpress_site: bool,

pub is_failed_to_parse_site_url: bool,
pub is_failed_to_parsed_api_root: bool,
pub is_failed_to_parsed_api_details: bool,
pub has_found_api_root_url: bool,
pub input_url: String,
pub parsed_site_url: Option<Arc<ParsedUrl>>,
pub api_root_url: Option<Arc<ParsedUrl>>,
pub api_details: Option<Arc<WpApiDetails>>,
// TODO: Not sure about the type for this
pub errors: u32,
}

pub fn construct_attempts(input_site_url: String) -> Vec<String> {
let mut attempts = vec![input_site_url.clone()];
if !input_site_url.starts_with("http") {
Expand Down
2 changes: 1 addition & 1 deletion wp_api_integration_tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ async-trait = { workspace = true }
clap = { workspace = true, features = ["derive"] }
futures = { workspace = true }
http = { workspace = true }
reqwest = { workspace = true, features = [ "multipart", "json", "stream" ] }
reqwest = { workspace = true, features = [ "multipart", "json", "stream", "gzip", "brotli", "zstd", "deflate" ] }
serde = { workspace = true, features = [ "derive" ] }
serde_json = { workspace = true }
tokio = { workspace = true, features = [ "full" ] }
Expand Down
18 changes: 18 additions & 0 deletions wp_api_integration_tests/tests/test_login_err.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use rstest::rstest;
use serial_test::parallel;
use std::sync::Arc;
use wp_api::login::{UrlDiscoveryError, WpLoginClient};
use wp_api_integration_tests::AsyncWpNetworking;

#[rstest]
#[case("http://optional-https.wpmt.co")] // Fails because it's `http`
#[tokio::test]
#[parallel]
async fn test_login_flow_err_url_discovery_failed(#[case] site_url: &str) {
let client = WpLoginClient::new(Arc::new(AsyncWpNetworking::default()));
let err = client
.api_discovery(site_url.to_string())
.await
.unwrap_err();
assert!(matches!(err, UrlDiscoveryError::UrlDiscoveryFailed { .. }));
}
15 changes: 15 additions & 0 deletions wp_api_integration_tests/tests/test_login_immut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use wp_api_integration_tests::{AssertResponse, AsyncWpNetworking};
const LOCALHOST_AUTH_URL: &str = "http://localhost/wp-admin/authorize-application.php";
const AUTOMATTIC_WIDGETS_AUTH_URL: &str =
"https://automatticwidgets.wpcomstaging.com/wp-admin/authorize-application.php";
const VANILLA_WP_SITE_URL: &str = "https://vanilla.wpmt.co/wp-admin/authorize-application.php";

#[rstest]
#[case("http://localhost", LOCALHOST_AUTH_URL)]
Expand Down Expand Up @@ -35,6 +36,20 @@ const AUTOMATTIC_WIDGETS_AUTH_URL: &str =
AUTOMATTIC_WIDGETS_AUTH_URL
)]
#[case("automatticwidgets.wpcomstaging.com/ ", AUTOMATTIC_WIDGETS_AUTH_URL)]
#[case("vanilla.wpmt.co", VANILLA_WP_SITE_URL)]
#[case("http://vanilla.wpmt.co", VANILLA_WP_SITE_URL)]
#[case(
"https://optional-https.wpmt.co",
"https://optional-https.wpmt.co/wp-admin/authorize-application.php"
)]
#[case(
"https://わぷー.wpmt.co",
"https://xn--39j4bws.wpmt.co/wp-admin/authorize-application.php"
)]
#[case(
"https://jetpack.wpmt.co",
"https://jetpack.wpmt.co/wp-admin/authorize-application.php"
)]
#[tokio::test]
#[parallel]
async fn test_login_flow(#[case] site_url: &str, #[case] expected_auth_url: &str) {
Expand Down
Loading