Page MenuHomePhabricator

[services][feature-flags] Introduce `features` endpoint
ClosedPublic

Authored by tomek on Feb 23 2023, 8:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 12:39 AM
Unknown Object (File)
Thu, Nov 28, 12:33 AM
Unknown Object (File)
Fri, Nov 15, 9:58 AM
Unknown Object (File)
Oct 26 2024, 10:12 PM
Unknown Object (File)
Oct 23 2024, 10:37 PM
Unknown Object (File)
Oct 21 2024, 1:28 AM
Unknown Object (File)
Oct 19 2024, 2:15 PM
Unknown Object (File)
Oct 18 2024, 5:42 PM
Subscribers

Details

Summary

Introduce a new endpoint /features that handles GET requests with query params containing platform, code_version and staff flag, e.g. /features?code_version=130&is_staff=false&platform=iOS.

Depends on D6860

Test Plan

Send a request to an endpoint and check if provided query params can be accessed.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Feb 23 2023, 8:58 AM
services/feature-flags/src/service.rs
17–20 ↗(On Diff #23015)

Do you have to clone it twice? I think the latter one can be just moved.

services/feature-flags/src/service.rs
84–87 ↗(On Diff #23015)

I would rather have #[allow(dead_code)] here than doing the "prefix with _" paradigm for structs.

Or come up with some other paradigm, but I would like to avoid intermediate diffs to bend over backwards for linting rules which won't apply in later diffs.

services/feature-flags/src/service.rs
84–87 ↗(On Diff #23015)

The #[allow(dead_code)] is good until you forget to remove it after the first usage

One option is to simply remove all occurrences in the last diff of the stack

services/feature-flags/src/service.rs
17–20 ↗(On Diff #23015)

I've tested it initially and only cloning twice worked for me, but I'll check once again

84–87 ↗(On Diff #23015)

In this case I think that @jon is right, as this struct is deserialized and prefixing might've broken something.

Rebase. Use to_owned instead of clone

services/feature-flags/src/service.rs
17–20 ↗(On Diff #23015)

I'm not 100% what's happening here, but if we remove the second copy we receive an error

error[E0507]: cannot move out of `db_clone`, a captured variable in an `Fn` closure
  --> src/service.rs:21:34
   |
18 |     let db_clone = self.db.clone();
   |         -------- captured outer variable
19 |     HttpServer::new(move || {
   |                     ------- captured by this `Fn` closure
20 |       App::new()
21 |         .app_data(web::Data::new(db_clone))
   |                                  ^^^^^^^^ move occurs because `db_clone` has type `DatabaseClient`, which does not implement the `Copy` trait

It might be caused by a fact that app_data expects <U: 'static>.

We can modify the code to be more explicit about moving here by using to_owned which usually uses clone inside.

bartek added inline comments.
services/feature-flags/src/service.rs
21 ↗(On Diff #23153)

Re: https://phab.comm.dev/D6861?id=23015#inline-45222

I'm not sure why captured variables aren't moved by default, but anyway the to_owned() looks like a correct solution.

This revision is now accepted and ready to land.Feb 27 2023, 5:43 AM