Page MenuHomePhabricator

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

Authored by tomek on Feb 23 2023, 8:43 AM.
Referenced Files
Unknown Object (File)
Thu, Jun 13, 2:11 AM
Unknown Object (File)
Thu, Jun 13, 12:13 AM
Unknown Object (File)
Sun, Jun 2, 5:42 AM
Unknown Object (File)
Tue, May 28, 6:28 AM
Unknown Object (File)
Sun, May 26, 6:34 PM
Unknown Object (File)
Sun, May 26, 1:52 PM
Unknown Object (File)
Sat, May 25, 10:18 PM
Unknown Object (File)
Thu, May 23, 4:13 PM



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Feb 23 2023, 8:58 AM
17–20 ↗(On Diff #23015)

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

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.

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

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

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/
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.
21 ↗(On Diff #23153)


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