trunk-merge/pr-1287/cbdbe1d6-87b7-42bb-85ef-143c5accebae

api/src/client/mod.rs

@@ -60,6 +60,7 @@ pub struct ApiClient {
     org_url_slug: String,
     render_sender: Option<Sender<DisplayMessage>>,
     upload_api_version: UploadApiVersion,
+    missing_test_collection: bool,
 }
 
 pub fn get_api_host() -> String {
@@ -221,9 +222,20 @@ impl ApiClient {
             org_url_slug,
             render_sender,
             upload_api_version: UploadApiVersion::V1,
+            missing_test_collection: false,
         })
     }
 
+    /// Trunk has no test collection for this run, so it makes no quarantine lookup and no upload:
+    /// v1 refuses both for a fully migrated org.
+    pub fn missing_test_collection(&self) -> bool {
+        self.missing_test_collection
+    }
+
+    pub fn set_missing_test_collection(&mut self, missing_test_collection: bool) {
+        self.missing_test_collection = missing_test_collection;
+    }
+
     pub fn upload_api_version(&self) -> UploadApiVersion {
         self.upload_api_version
     }
@@ -236,29 +248,36 @@ impl ApiClient {
     /// Asks Trunk whether this run uses v2. Anything but an explicit `v2`, a failed call
     /// included, is v1, so this never fails a run. A timeout, a connection error or a 5xx is
     /// retried once; any other answer is final.
+    ///
+    /// A run that names no collection asks too, by its repository: Trunk may answer `v2` with the
+    /// collection it inferred. A `v2` that names none is v1, since every v2 route needs one. A `v1`
+    /// with `missingTestCollection` means Trunk has no collection for the run at all.
     pub async fn get_upload_api_version(
         &self,
         request: &message::GetUploadApiVersionRequest,
-    ) -> UploadApiVersion {
-        let upload_api_version = if self.trunk_api_v2_client.is_none()
-            || request
-                .test_collection_short_id
-                .as_deref()
-                .is_none_or(str::is_empty)
-        {
-            UploadApiVersion::V1
+    ) -> message::UploadApiDecision {
+        let requested = request
+            .test_collection_short_id
+            .as_deref()
+            .filter(|id| !id.trim().is_empty());
+        let has_repo = request
+            .repo
+            .as_ref()
+            .and_then(message::named_repo)
+            .is_some();
+        let decision = if self.trunk_api_v2_client.is_none() || (requested.is_none() && !has_repo) {
+            message::UploadApiDecision::default()
         } else {
             match self.request_upload_api_version(request).await {
-                Ok(response) if response.api_version == "v2" => UploadApiVersion::V2,
-                Ok(_) => UploadApiVersion::V1,
+                Ok(response) => message::UploadApiDecision::from_response(response, requested),
                 Err(error) => {
                     tracing::debug!("Could not choose the Trunk API version, using v1: {error:?}");
-                    UploadApiVersion::V1
+                    message::UploadApiDecision::default()
                 }
             }
         };
-        tracing::info!("Using Trunk API {}", upload_api_version.as_str());
-        upload_api_version
+        tracing::info!("Using Trunk API {}", decision.version.as_str());
+        decision
     }
 
     async fn request_upload_api_version(

api/src/message.rs

@@ -119,6 +119,92 @@ pub struct GetUploadApiVersionRequest {
 #[serde(rename_all = "camelCase")]
 pub struct GetUploadApiVersionResponse {
     pub api_version: String,
+    /// Sent only with `v2`, for a run that named no collection, when Trunk inferred one from the
+    /// repository. Older servers never send it.
+    #[serde(default)]
+    pub test_collection_short_id: Option<String>,
+    /// Sent only with `v1`, for a fully migrated org's run that named no collection when Trunk
+    /// could not infer one. Trunk would drop a v1 upload of that run. Older servers never send it.
+    #[serde(default)]
+    pub missing_test_collection: bool,
+}
+
+/// Which API a run uses and, on v2 for a run that named no test collection, the collection Trunk
+/// inferred from its repository.
+#[derive(Debug, Clone, PartialEq, Eq, Default)]
+pub struct UploadApiDecision {
+    pub version: UploadApiVersion,
+    pub inferred_test_collection_short_id: Option<String>,
+    /// The run named no collection and Trunk has none for it, so the upload is skipped.
+    pub missing_test_collection: bool,
+}
+
+impl UploadApiDecision {
+    /// A run that named its collection takes the answer's version as it is. Only a run that named
+    /// none reads the other fields: the inferred collection with `v2`, `missingTestCollection` with
+    /// `v1`.
+    pub fn from_response(response: GetUploadApiVersionResponse, requested: Option<&str>) -> Self {
+        let v2 = response.api_version == "v2";
+        if requested.is_some_and(|id| !id.trim().is_empty()) {
+            return Self::from(if v2 {
+                UploadApiVersion::V2
+            } else {
+                UploadApiVersion::V1
+            });
+        }
+        if !v2 {
+            return Self {
+                missing_test_collection: response.missing_test_collection,
+                ..Self::default()
+            };
+        }
+        match response
+            .test_collection_short_id
+            .filter(|id| !id.trim().is_empty())
+        {
+            Some(inferred) => Self {
+                version: UploadApiVersion::V2,
+                inferred_test_collection_short_id: Some(inferred),
+                ..Self::default()
+            },
+            None => {
+                tracing::debug!(
+                    "Trunk chose v2 but named no test collection for this run, using v1"
+                );
+                Self::default()
+            }
+        }
+    }
+
+    /// The inferred collection, when the run named none of its own and so uses it.
+    pub fn inferred_for(&self, requested: Option<&str>) -> Option<&str> {
+        match (self.version, requested.filter(|id| !id.trim().is_empty())) {
+            (UploadApiVersion::V2, None) => self
+                .inferred_test_collection_short_id
+                .as_deref()
+                .filter(|id| !id.trim().is_empty()),
+            _ => None,
+        }
+    }
+
+    /// Every v2 route is scoped to a collection, so v2 with none named and none inferred is v1.
+    pub fn version_for(&self, requested: Option<&str>) -> UploadApiVersion {
+        let has_collection = requested.is_some_and(|id| !id.trim().is_empty())
+            || self.inferred_for(requested).is_some();
+        match self.version {
+            UploadApiVersion::V2 if has_collection => UploadApiVersion::V2,
+            _ => UploadApiVersion::V1,
+        }
+    }
+}
+
+impl From<UploadApiVersion> for UploadApiDecision {
+    fn from(version: UploadApiVersion) -> Self {
+        Self {
+            version,
+            ..Self::default()
+        }
+    }
 }
 
 #[derive(Debug, Serialize, Clone, PartialEq, Eq)]

api/tests/v2_client.rs

@@ -1,7 +1,7 @@
 use std::{collections::HashSet, time::Duration};
 
 use api::client::ApiClient;
-use api::message::{self, UploadApiCaller, UploadApiVersion};
+use api::message::{self, UploadApiCaller, UploadApiDecision, UploadApiVersion};
 use api::quarantine::{QuarantineLookup, QuarantinedTestSet, QuarantinedTests};
 use api::urls::TestCaseGuidScope;
 use axum::{
@@ -131,6 +131,14 @@ fn scope() -> TestCaseGuidScope {
 }
 
 async fn decide(answer: (StatusCode, &'static str)) -> (UploadApiVersion, SharedMockServerState) {
+    let (decision, state) = decide_for(Some(SHORT_ID), answer).await;
+    (decision.version, state)
+}
+
+async fn decide_for(
+    short_id: Option<&str>,
+    answer: (StatusCode, &'static str),
+) -> (UploadApiDecision, SharedMockServerState) {
     let state = serve(vec![(
         VERSION_ROUTE,
         post(
@@ -144,10 +152,10 @@ async fn decide(answer: (StatusCode, &'static str)) -> (UploadApiVersion, Shared
         ),
     )])
     .await;
-    let version = client(&state, Some("org-key"))
-        .get_upload_api_version(&version_request(Some(SHORT_ID)))
+    let decision = client(&state, Some("org-key"))
+        .get_upload_api_version(&version_request(short_id))
         .await;
-    (version, state)
+    (decision, state)
 }
 
 fn quarantine_status_route(
@@ -279,7 +287,8 @@ async fn retries_a_failed_check_once() {
 
     let version = client(&state, Some("org-key"))
         .get_upload_api_version(&version_request(Some(SHORT_ID)))
-        .await;
+        .await
+        .version;
 
     assert_eq!((version, uris(&state).len()), (UploadApiVersion::V2, 2));
 }
@@ -297,19 +306,20 @@ async fn reads_a_slow_answer_as_v1() {
 
     let version = client(&state, Some("org-key"))
         .get_upload_api_version(&version_request(Some(SHORT_ID)))
-        .await;
+        .await
+        .version;
 
     assert_eq!(version, UploadApiVersion::V1);
 }
 
 #[tokio::test]
-async fn does_not_ask_without_a_token_or_a_collection() {
+async fn does_not_ask_without_a_token_or_a_collection_and_a_repo() {
     let state = serve(vec![(
         VERSION_ROUTE,
         post(
             |State(state): State<SharedMockServerState>, headers: HeaderMap, uri: Uri| async move {
                 state.record_route_request(&headers, &uri, None);
-                Json(json!({"apiVersion": "v2"}))
+                Json(json!({"apiVersion": "v2", "testCollectionShortId": SHORT_ID}))
             },
         ),
     )])
@@ -322,19 +332,104 @@ async fn does_not_ask_without_a_token_or_a_collection() {
         public_repo_client
             .get_upload_api_version(&version_request(Some(SHORT_ID)))
             .await,
-        UploadApiVersion::V1
+        UploadApiDecision::default()
     );
-    for short_id in [None, Some("")] {
+    // Without a collection, only the repository lets Trunk infer one.
+    for repo in [
+        None,
+        Some(context::repo::RepoUrlParts {
+            name: String::new(),
+            ..repo()
+        }),
+    ] {
+        let request = message::GetUploadApiVersionRequest {
+            repo,
+            ..version_request(None)
+        };
         assert_eq!(
             client(&state, Some("org-key"))
-                .get_upload_api_version(&version_request(short_id))
+                .get_upload_api_version(&request)
                 .await,
-            UploadApiVersion::V1
+            UploadApiDecision::default()
         );
     }
     assert!(uris(&state).is_empty());
 }
 
+#[tokio::test]
+async fn uses_the_collection_inferred_for_a_run_without_one() {
+    for short_id in [None, Some("")] {
+        let (decision, state) = decide_for(
+            short_id,
+            (
+                StatusCode::OK,
+                r#"{"apiVersion":"v2","testCollectionShortId":"zQ8nP2mW","futureField":1}"#,
+            ),
+        )
+        .await;
+
+        assert_eq!(
+            decision,
+            UploadApiDecision {
+                version: UploadApiVersion::V2,
+                inferred_test_collection_short_id: Some(String::from("zQ8nP2mW")),
+                missing_test_collection: false,
+            }
+        );
+        assert_eq!(
+            bodies(&state)[0]["repo"],
+            json!({"host": "github.com", "owner": "acme", "name": "web"})
+        );
+    }
+}
+
+#[tokio::test]
+async fn reads_a_missing_collection_for_a_run_without_one() {
+    let answer = r#"{"apiVersion":"v1","missingTestCollection":true}"#;
+    for short_id in [None, Some("")] {
+        let (decision, _) = decide_for(short_id, (StatusCode::OK, answer)).await;
+        assert_eq!(
+            decision,
+            UploadApiDecision {
+                version: UploadApiVersion::V1,
+                inferred_test_collection_short_id: None,
+                missing_test_collection: true,
+            },
+            "{short_id:?}"
+        );
+    }
+    let (decision, _) = decide_for(Some(SHORT_ID), (StatusCode::OK, answer)).await;
+    assert_eq!(decision, UploadApiDecision::default());
+}
+
+#[tokio::test]
+async fn reads_v2_without_a_collection_as_v1() {
+    for answer in [
+        r#"{"apiVersion":"v2"}"#,
+        r#"{"apiVersion":"v2","testCollectionShortId":null}"#,
+        r#"{"apiVersion":"v2","testCollectionShortId":" "}"#,
+        r#"{"apiVersion":"v1","testCollectionShortId":"zQ8nP2mW"}"#,
+    ] {
+        let (decision, state) = decide_for(None, (StatusCode::OK, answer)).await;
+        assert_eq!(decision, UploadApiDecision::default(), "{answer}");
+        assert_eq!(bodies(&state).len(), 1, "{answer}");
+    }
+}
+
+#[tokio::test]
+async fn ignores_an_inferred_collection_when_the_run_names_one() {
+    let (decision, _) = decide_for(
+        Some(SHORT_ID),
+        (
+            StatusCode::OK,
+            r#"{"apiVersion":"v2","testCollectionShortId":"zQ8nP2mW"}"#,
+        ),
+    )
+    .await;
+
+    assert_eq!(decision, UploadApiDecision::from(UploadApiVersion::V2));
+}
+
 #[tokio::test]
 async fn creates_a_v2_upload() {
     let state = serve(vec![(