add request context helper

yuka commented

2024-03-03 13:59:19 +00:00

Contributor

As suggested by LX, add a request context helper which contains a bunch of variables which are passed to almost all API request handlers.

Additionally, limit bucket.state.as_option.unwrap() occurrences to only directly after the get_existing_bucket. Since the api_server already does get_existing_bucket, it knows for sure that the bucket exists in a non-deleted state, so it can unwrap the bucket state/params and pass them through the request context, making the unwrap() in the request handler unnecessary.

As suggested by LX, add a request context helper which contains a bunch of variables which are passed to almost all API request handlers. Additionally, limit bucket.state.as_option.unwrap() occurrences to only directly after the get_existing_bucket. Since the api_server already does get_existing_bucket, it knows for sure that the bucket exists in a non-deleted state, so it can unwrap the bucket state/params and pass them through the request context, making the unwrap() in the request handler unnecessary.

yuka force-pushed req-ctx from 3b00c43e15 to 4a354a5735

2024-03-03 14:04:39 +00:00

Compare

yuka force-pushed req-ctx from 4a354a5735 to daee9b75c4

2024-03-03 14:06:42 +00:00

Compare

yuka force-pushed req-ctx from daee9b75c4 to 7b516ef3c4

2024-03-03 14:11:46 +00:00

Compare

lx commented

2024-03-03 14:26:18 +00:00

Owner

Does this patch apply easily to the main (v0.9) branch ? Because if it does it would be nice to have it there, to minimize divergence between the two.

yuka force-pushed req-ctx from 7b516ef3c4 to c1b6fd48bc

2024-03-03 14:38:40 +00:00

Compare

yuka changed target branch from next-0.10 to main

2024-03-03 14:38:45 +00:00

yuka commented

2024-03-03 14:38:59 +00:00

Author

Contributor

Rebased and changed the target branch to main

yuka commented

2024-03-03 16:16:13 +00:00

Author

Contributor

I wonder if the hyper Request should also be made a part of the request context

lx commented

2024-03-04 11:05:52 +00:00

Owner

I think we should avoid adding the Request in there for now, I'm afraid it might add more noise and not really help refactor anything.

The objetive of this diff is to make the BucketParams available everywhere in the functions that handle API paths, because this will be necessary to have per-bucket consistency levels, and later on, different storage pools. In its current form, I find this patch not very satisfying because it seems to add more lines/noise than it actually avoids, but later on we can expect that it will be usefull.

There are maybe two things I would like to ask you to do before merging:

Uniformize the convention for receiving a ReqCtx in all of the handler conventions. I've seen three patterns throughout the code: destructure the ReqCtx in the list of arguments (e.g. handle_create_multipart_upload), destructure the ReqCtx at the beginning of the function (e.g. handle_put_part), and have a separate funciton that destructures the ReqCtx and calls the original function (e.g. handle_head). My recommendation would be to use the second pattern (destructure ReqCtx using a let binding at the beginning of the handler function) for all requests.
I don't like that we are writing this kind of things in all of the functions that modify the bucket parameters:

	let bucket = Bucket {
		id: bucket_id,
		state: Deletable::Present(bucket_params),
	};

I think we should add a helper to the Bucket struct in bucket_table.rs so that we can just write Bucket::present(bucket_id, bucket_params).

Thanks :)

I think we should avoid adding the Request in there for now, I'm afraid it might add more noise and not really help refactor anything. The objetive of this diff is to make the BucketParams available everywhere in the functions that handle API paths, because this will be necessary to have per-bucket consistency levels, and later on, different storage pools. In its current form, I find this patch not very satisfying because it seems to add more lines/noise than it actually avoids, but later on we can expect that it will be usefull. There are maybe two things I would like to ask you to do before merging: 1. Uniformize the convention for receiving a ReqCtx in all of the handler conventions. I've seen three patterns throughout the code: destructure the ReqCtx in the list of arguments (e.g. `handle_create_multipart_upload`), destructure the ReqCtx at the beginning of the function (e.g. `handle_put_part`), and have a separate funciton that destructures the ReqCtx and calls the original function (e.g. `handle_head`). My recommendation would be to use the second pattern (destructure ReqCtx using a let binding at the beginning of the handler function) for all requests. 2. I don't like that we are writing this kind of things in all of the functions that modify the bucket parameters: ```rust let bucket = Bucket { id: bucket_id, state: Deletable::Present(bucket_params), }; ``` I think we should add a helper to the Bucket struct in bucket_table.rs so that we can just write `Bucket::present(bucket_id, bucket_params)`. Thanks :)

yuka commented

2024-03-04 11:58:26 +00:00

Author

Contributor

Uniformize the convention for receiving a ReqCtx in all of the handler conventions. I've seen three patterns throughout the code:
destructure the ReqCtx in the list of arguments (e.g. handle_create_multipart_upload),

Oops, this is what I did in the beginning, but then changed to the second pattern in case the whole context needs to be passed to a helper function.

destructure the ReqCtx at the beginning of the function (e.g. handle_put_part), and

This is what I do now in all cases where it is possible.

have a separate funciton that destructures the ReqCtx and calls the original function (e.g. handle_head).

This has a specific reason: The web_server.rs calls handle_head without providing an access/api key. So the handle_head_without_ctx is also used directly from elsewhere.

My recommendation would be to use the second pattern (destructure ReqCtx using a let binding at the beginning of the handler function) for all requests.

This was also my intent with the exception seen above.

> Uniformize the convention for receiving a ReqCtx in all of the handler conventions. I've seen three patterns throughout the code: > destructure the ReqCtx in the list of arguments (e.g. handle_create_multipart_upload), Oops, this is what I did in the beginning, but then changed to the second pattern in case the whole context needs to be passed to a helper function. > destructure the ReqCtx at the beginning of the function (e.g. handle_put_part), and This is what I do now in all cases where it is possible. > have a separate funciton that destructures the ReqCtx and calls the original function (e.g. handle_head). This has a specific reason: The web_server.rs calls handle_head without providing an access/api key. So the handle_head_without_ctx is also used directly from elsewhere. > My recommendation would be to use the second pattern (destructure ReqCtx using a let binding at the beginning of the handler function) for all requests. This was also my intent with the exception seen above.

yuka force-pushed req-ctx from c1b6fd48bc to 639f4d4b0f

2024-03-04 12:08:17 +00:00

Compare

yuka force-pushed req-ctx from 639f4d4b0f to fb55682c66

2024-03-04 12:26:46 +00:00

Compare

yuka commented

2024-03-04 12:26:56 +00:00

Author

Contributor

✅ Implemented the suggestions as far as possible