Skip to content

Commit

Permalink
Update HTTP verbs for DAP-13. (#3469)
Browse files Browse the repository at this point in the history 
Specifically:
* Use POST rather than PUT for report uploads.
* Use GET rather than POST for collection job polling.

Also, disable divviup-ts integration tests:

Until divviup-ts has a DAP-13 implementation, there is no way these
tests will work. I filed #3479 to track re-enablement of divviup-ts
tests.
  • Loading branch information
@branlwyd
branlwyd authored Nov 13, 2024
1 parent bf396e0 commit f15ea04
Show file tree
Hide file tree
Showing 11 changed files with 92 additions and 92 deletions.
30 changes: 13 additions & 17 deletions aggregator/src/aggregator/collection_job_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use std::{collections::HashSet, sync::Arc};
use trillium::{Handler, KnownHeaderName, Status};
use trillium_testing::{
assert_headers,
prelude::{post, put},
prelude::{get, put},
TestConn,
};

Expand Down Expand Up @@ -95,29 +95,25 @@ impl CollectionJobTestCase {
.await
}

pub(super) async fn post_collection_job_with_auth_token(
pub(super) async fn get_collection_job_with_auth_token(
&self,
collection_job_id: &CollectionJobId,
auth_token: Option<&AuthenticationToken>,
) -> TestConn {
let mut test_conn = post(
self.task
.collection_job_uri(collection_job_id)
.unwrap()
.path(),
);
let mut test_conn = get(self
.task
.collection_job_uri(collection_job_id)
.unwrap()
.path());
if let Some(auth) = auth_token {
let (header, value) = auth.request_authentication();
test_conn = test_conn.with_request_header(header, value);
}
test_conn.run_async(&self.handler).await
}

pub(super) async fn post_collection_job(
&self,
collection_job_id: &CollectionJobId,
) -> TestConn {
self.post_collection_job_with_auth_token(
pub(super) async fn get_collection_job(&self, collection_job_id: &CollectionJobId) -> TestConn {
self.get_collection_job_with_auth_token(
collection_job_id,
Some(self.task.collector_auth_token()),
)
Expand Down Expand Up @@ -343,7 +339,7 @@ async fn collection_job_success_fixed_size() {
.await;
assert_eq!(test_conn.status(), Some(Status::Created));

let test_conn = test_case.post_collection_job(&collection_job_id).await;
let test_conn = test_case.get_collection_job(&collection_job_id).await;
assert_eq!(test_conn.status(), Some(Status::Accepted));

// Update the collection job with the aggregate shares. collection job should now be complete.
Expand Down Expand Up @@ -408,7 +404,7 @@ async fn collection_job_success_fixed_size() {
panic!("unexpected batch ID");
}

let mut test_conn = test_case.post_collection_job(&collection_job_id).await;
let mut test_conn = test_case.get_collection_job(&collection_job_id).await;
assert_headers!(&test_conn, "content-type" => (Collection::<FixedSize>::MEDIA_TYPE));

let collect_resp: Collection<FixedSize> = decode_response_body(&mut test_conn).await;
Expand Down Expand Up @@ -817,7 +813,7 @@ async fn collection_job_put_idempotence_fixed_size_current_batch_no_extra_report
assert_eq!(response.status(), Some(Status::Created));

// Fetch the first collection job, to advance the current batch.
let response = test_case.post_collection_job(&collection_job_id_1).await;
let response = test_case.get_collection_job(&collection_job_id_1).await;
assert_eq!(response.status(), Some(Status::Accepted));

// Create the second collection job.
Expand All @@ -828,7 +824,7 @@ async fn collection_job_put_idempotence_fixed_size_current_batch_no_extra_report

// Fetch the second collection job, to advance the current batch. There are now no outstanding
// batches left.
let response = test_case.post_collection_job(&collection_job_id_2).await;
let response = test_case.get_collection_job(&collection_job_id_2).await;
assert_eq!(response.status(), Some(Status::Accepted));

// Re-send the collection job creation requests to confirm they are still idempotent.
Expand Down
16 changes: 8 additions & 8 deletions aggregator/src/aggregator/http_handlers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ where
"hpke_config",
hpke_config_cors_preflight,
)
.put("tasks/:task_id/reports", instrumented(api(upload::<C>)))
.post("tasks/:task_id/reports", instrumented(api(upload::<C>)))
.with_route(
trillium::Method::Options,
"tasks/:task_id/reports",
Expand Down Expand Up @@ -397,9 +397,9 @@ where
COLLECTION_JOB_ROUTE,
instrumented(api(collection_jobs_put::<C>)),
)
.post(
.get(
COLLECTION_JOB_ROUTE,
instrumented(api(collection_jobs_post::<C>)),
instrumented(api(collection_jobs_get::<C>)),
)
.delete(
COLLECTION_JOB_ROUTE,
Expand Down Expand Up @@ -491,7 +491,7 @@ async fn hpke_config_cors_preflight(mut conn: Conn) -> Conn {
conn
}

/// API handler for the "/tasks/.../reports" PUT endpoint.
/// API handler for the "/tasks/.../reports" POST endpoint.
async fn upload<C: Clock>(
conn: &mut Conn,
(State(aggregator), BodyBytes(body)): (State<Arc<Aggregator<C>>>, BodyBytes),
Expand All @@ -517,12 +517,12 @@ async fn upload<C: Clock>(
/// Handler for CORS preflight requests to "/tasks/.../reports".
async fn upload_cors_preflight(mut conn: Conn) -> Conn {
conn.response_headers_mut()
.insert(KnownHeaderName::Allow, "PUT");
.insert(KnownHeaderName::Allow, "POST");
if let Some(origin) = conn.request_headers().get(KnownHeaderName::Origin) {
let origin = origin.clone();
let request_headers = conn.response_headers_mut();
request_headers.insert(KnownHeaderName::AccessControlAllowOrigin, origin);
request_headers.insert(KnownHeaderName::AccessControlAllowMethods, "PUT");
request_headers.insert(KnownHeaderName::AccessControlAllowMethods, "POST");
request_headers.insert(KnownHeaderName::AccessControlAllowHeaders, "content-type");
request_headers.insert(
KnownHeaderName::AccessControlMaxAge,
Expand Down Expand Up @@ -629,8 +629,8 @@ async fn collection_jobs_put<C: Clock>(
Ok(Status::Created)
}

/// API handler for the "/tasks/.../collection_jobs/..." POST endpoint.
async fn collection_jobs_post<C: Clock>(
/// API handler for the "/tasks/.../collection_jobs/..." GET endpoint.
async fn collection_jobs_get<C: Clock>(
conn: &mut Conn,
State(aggregator): State<Arc<Aggregator<C>>>,
) -> Result<(), Error> {
Expand Down
20 changes: 10 additions & 10 deletions aggregator/src/aggregator/http_handlers/tests/collection_job.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use serde_json::json;
use trillium::{KnownHeaderName, Status};
use trillium_testing::{
assert_headers,
prelude::{delete, post, put},
prelude::{delete, get, put},
};

#[tokio::test]
Expand Down Expand Up @@ -262,7 +262,7 @@ async fn collection_job_put_request_unauthenticated() {
}

#[tokio::test]
async fn collection_job_post_request_unauthenticated_collection_jobs() {
async fn collection_job_get_request_unauthenticated_collection_jobs() {
let test_case = setup_collection_job_test_case(Role::Leader, QueryType::TimeInterval).await;
test_case
.setup_time_interval_batch(Time::from_seconds_since_epoch(0))
Expand All @@ -288,7 +288,7 @@ async fn collection_job_post_request_unauthenticated_collection_jobs() {

// Incorrect authentication token.
let mut test_conn = test_case
.post_collection_job_with_auth_token(&collection_job_id, Some(&random()))
.get_collection_job_with_auth_token(&collection_job_id, Some(&random()))
.await;

let want_status = u16::from(Status::Forbidden);
Expand All @@ -305,7 +305,7 @@ async fn collection_job_post_request_unauthenticated_collection_jobs() {

// Aggregator authentication token.
let mut test_conn = test_case
.post_collection_job_with_auth_token(
.get_collection_job_with_auth_token(
&collection_job_id,
Some(test_case.task.aggregator_auth_token()),
)
Expand All @@ -325,7 +325,7 @@ async fn collection_job_post_request_unauthenticated_collection_jobs() {

// Missing authentication token.
let mut test_conn = test_case
.post_collection_job_with_auth_token(&collection_job_id, None)
.get_collection_job_with_auth_token(&collection_job_id, None)
.await;

let want_status = u16::from(Status::Forbidden);
Expand Down Expand Up @@ -398,7 +398,7 @@ async fn collection_job_success_time_interval() {

assert_eq!(test_conn.status(), Some(Status::Created));

let test_conn = test_case.post_collection_job(&collection_job_id).await;
let test_conn = test_case.get_collection_job(&collection_job_id).await;
assert_eq!(test_conn.status(), Some(Status::Accepted));

// Update the collection job with the aggregate shares and some aggregation jobs. collection
Expand Down Expand Up @@ -452,7 +452,7 @@ async fn collection_job_success_time_interval() {
.await
.unwrap();

let mut test_conn = test_case.post_collection_job(&collection_job_id).await;
let mut test_conn = test_case.get_collection_job(&collection_job_id).await;

assert_eq!(test_conn.status(), Some(Status::Ok));
assert_headers!(
Expand Down Expand Up @@ -502,7 +502,7 @@ async fn collection_job_success_time_interval() {
}

#[tokio::test]
async fn collection_job_post_request_no_such_collection_job() {
async fn collection_job_get_request_no_such_collection_job() {
let test_case = setup_collection_job_test_case(Role::Leader, QueryType::TimeInterval).await;
test_case
.setup_time_interval_batch(Time::from_seconds_since_epoch(0))
Expand All @@ -514,7 +514,7 @@ async fn collection_job_post_request_no_such_collection_job() {
.task
.collector_auth_token()
.request_authentication();
let test_conn = post(format!(
let test_conn = get(format!(
"/tasks/{}/collection_jobs/{no_such_collection_job_id}",
test_case.task.id()
))
Expand Down Expand Up @@ -659,6 +659,6 @@ async fn delete_collection_job() {
assert_eq!(test_conn.status(), Some(Status::NoContent));

// Get the job again
let test_conn = test_case.post_collection_job(&collection_job_id).await;
let test_conn = test_case.get_collection_job(&collection_job_id).await;
assert_eq!(test_conn.status(), Some(Status::NoContent));
}
Loading

0 comments on commit f15ea04

Please sign in to comment.