Page MenuHomePhabricator
Differential D6862 Diff 23016 services/feature-flags/src/service.rs

Changeset View

Standalone View

View Options

services/feature-flags/src/service.rs

Show All 23 Lines HttpServer::new(move || {
) )
}) })
.bind(("127.0.0.1", CONFIG.http_port))? .bind(("127.0.0.1", CONFIG.http_port))?
.run() .run()
.await .await
} }
async fn features_handler( async fn features_handler(
_client: web::Data<DatabaseClient>, client: web::Data<DatabaseClient>,
_query: web::Query<FeatureQuery>, query: web::Query<FeatureQuery>,
) -> Result<HttpResponse, actix_web::Error> { ) -> HttpResponse {
Ok(HttpResponse::Ok().body("HELLO")) let platform = match query.platform.as_str() {
"iOS" => Some(Platform::IOS),
"Android" => Some(Platform::ANDROID),
bartekUnsubmitted
Not Done
Inline Actions

Is it case-sensitive? And should URLs be?

bartek: Is it case-sensitive? And should URLs be?
tomekAuthorUnsubmitted
Done
Inline Actions

Query part of url is case-sensitive. I guess we can make the check on service side case-insensitive, just for the convenience.

tomek: Query part of url is case-sensitive. I guess we can make the check on service side case…
_ => None,
};
match platform {
Some(p) => match Self::enabled_features_set(
client.get_ref(),
p,
query.code_version,
query.is_staff,
)
.await
{
Ok(features) => HttpResponse::Ok()
.body(features.into_iter().collect::<Vec<_>>().join(", ")),
bartekUnsubmitted
Not Done
Inline Actions
  • It'd be easier to parse if the separator didn't contain whitespaces, so just a comma or semicolon
  • Is the response Content-Type automatically set to text/plain?
bartek: - It'd be easier to parse if the separator didn't contain whitespaces, so just a comma or…
tomekAuthorUnsubmitted
Done
Inline Actions

Makes sense!

The docs don't mention that the content is set automatically, so I added it explicitly.

tomek: Makes sense! The docs don't mention that the content is set automatically, so I added it…
bartekUnsubmitted
Not Done
Inline Actions
.await
{
- Ok(features) => HttpResponse::Ok()
- .body(features.into_iter().collect::<Vec<_>>().join(", ")),
+ Ok(features) => {
+ let response_body = features.into_iter().collect::<Vec<_>>().join(", ");
+ HttpResponse::Ok().body(response_body)
+ },
_ => HttpResponse::InternalServerError().finish(),

I'm wondering if this isn't more readable than a one-liner. Up to you

bartek: I'm wondering if this isn't more readable than a one-liner. Up to you
tomekAuthorUnsubmitted
Done
Inline Actions

Agree, it's more readable.

tomek: Agree, it's more readable.
_ => HttpResponse::InternalServerError().finish(),
},
None => HttpResponse::BadRequest().finish(),
}
bartekUnsubmitted
Not Done
Inline Actions

I'd prefer to return early (pasted code here instead of suggestion because it was so unreadable)

let platform = match query.platform.as_str() {
  "iOS" => Platform::IOS,
  "Android" => Platform::ANDROID,
  _ => {
    return HttpResponse::BadRequest().finish()
  },
};
match Self::enabled_features_set(
  client.get_ref(),
  platform,
  query.code_version,
  query.is_staff,
)
.await
{
  Ok(features) => HttpResponse::Ok()
    .body(features.into_iter().collect::<Vec<_>>().join(", ")),
  _ => HttpResponse::InternalServerError().finish(),
}
bartek: I'd prefer to return early //(pasted code here instead of suggestion because it was so…
} }
async fn _enabled_features_set( async fn enabled_features_set(
db: &DatabaseClient, db: &DatabaseClient,
platform: Platform, platform: Platform,
code_version: i32, code_version: i32,
is_staff: bool, is_staff: bool,
) -> Result<HashSet<String>, Error> { ) -> Result<HashSet<String>, Error> {
let features_config = db.get_features_configuration(platform).await?; let features_config = db.get_features_configuration(platform).await?;
Ok( Ok(
features_config features_config
.into_values() .into_values()
.filter_map(|config| { .filter_map(|config| {
Self::_check_if_feature_is_enabled(code_version, is_staff, config) Self::check_if_feature_is_enabled(code_version, is_staff, config)
}) })
.collect(), .collect(),
) )
} }
fn _check_if_feature_is_enabled( fn check_if_feature_is_enabled(
code_version: i32, code_version: i32,
is_staff: bool, is_staff: bool,
feature_config: FeatureConfig, feature_config: FeatureConfig,
) -> Option<String> { ) -> Option<String> {
feature_config feature_config
.config .config
.keys() .keys()
.filter(|version| version.clone() <= &code_version) .filter(|version| version.clone() <= &code_version)
Show All 13 Lines feature_config
None None
} }
}) })
} }
} }
#[derive(Deserialize, Debug)] #[derive(Deserialize, Debug)]
struct FeatureQuery { struct FeatureQuery {
_code_version: i32, code_version: i32,
_is_staff: bool, is_staff: bool,
_platform: String, platform: String,
} }