Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mobile config] Add v2 endpoints to GatewayInfoV2. Change min_refreshed_at to min_updated_at #910

Merged
merged 11 commits into from
Dec 17, 2024

Conversation

kurotych
Copy link
Member

@kurotych kurotych commented Dec 9, 2024

After #902 was implemented, there is now a last_changed_at field, which better fits the purpose compared to refreshed_at, as last_changed_at signals radio changes.

This breaks info_stream_v2 backward compatibility, but it is expected that no clients are using v2 yet.
Related PR: helium/proto#434

UPD: Added appropriate v2 endpoints that returns GatewayInfoV2 which uses v1 request version since nothing (from request side) has changed

@kurotych kurotych force-pushed the min_update_at branch 2 times, most recently from 2898d4b to 7393d81 Compare December 9, 2024 15:24
@kurotych kurotych marked this pull request as ready for review December 9, 2024 15:33
Comment on lines 386 to 397
let rows: Vec<Vec<u8>> = sqlx::query_scalar(GET_UPDATED_RADIOS)
.bind(min_updated_at)
.fetch_all(db)
.await?;
let mut radios = HashSet::new();

for row in rows {
let entity_key_b: &[u8] = &row;
let entity_key = bs58::encode(entity_key_b).into_string();
let pk = PublicKeyBinary::from_str(&entity_key)?;
radios.insert(pk);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let rows: Vec<Vec<u8>> = sqlx::query_scalar(GET_UPDATED_RADIOS)
.bind(min_updated_at)
.fetch_all(db)
.await?;
let mut radios = HashSet::new();
for row in rows {
let entity_key_b: &[u8] = &row;
let entity_key = bs58::encode(entity_key_b).into_string();
let pk = PublicKeyBinary::from_str(&entity_key)?;
radios.insert(pk);
}
let radios: HashSet<PublicKeyBinary> = sqlx::query_scalar(GET_UPDATED_RADIOS)
.bind(min_updated_at)
.fetch(db)
.await
.map(|row| {
let entity_key_b: &[u8] = &row;
let entity_key = bs58::encode(entity_key_b).into_string();
PublicKeyBinary::from_str(&entity_key)
})
.collect::<Result<HashSet<PublicKeyBinary>, helium_crypto::Error>>()?;

this probably necessitates importing StreamExt or similar but avoids the extra allocations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

helium-crypto-rs provides a sqlx mapping for PublicKeyBinary https://github.com/helium/helium-crypto-rs/blob/main/src/public_key_binary.rs#L115

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @michaeldjeffrey thanks for information. The helium-crypto-rs provides a sqlx mapping for PublicKeyBinary which stored in database as string. In our case pubkey is stored as a bytea type. So when I try to use helium-crypto-rs
mapping like below

Ok(sqlx::query_as::<_, (PublicKeyBinary,)>(GET_UPDATED_RADIOS)
           .bind(min_updated_at)
           .fetch(db)
           .map_ok(|(entity_key,)| entity_key)
           .try_collect::<HashSet<_>>()
           .await?)

I've got an error: mismatched types; Rust type `helium_crypto::public_key_binary::PublicKeyBinary` (as SQL type `text`) is not compatible with SQL type BYTEA

To make it work we need to extend decode function in helium-crypto-rs https://github.com/helium/helium-crypto-rs/blob/main/src/public_key_binary.rs#L150 to make it support bytea. I think this is not in the scope of this PR and I'm not sure that it is worth since I have no information how often we store PublicKeyBinary in bytea type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No doubt. That's a good @madninja question. Anyways, carry on 👯

@@ -191,27 +198,39 @@ impl mobile_config::Gateway for GatewayService {
self.verify_request_signature(&signer, &request)?;

let pool = self.metadata_pool.clone();
let mc_pool = self.mobile_config_db_pool.clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original pool variable name made sense here as-is when it was the only database connection this function needed but in general i'd lean more towards the app's "owned" database connection more generically and the one "owned" by an outside service (the metadata db in this case) with more specificity the way we do in the config service's settings or else just give them both more specific names to avoid any confusion

Comment on lines 215 to 233
let stream = gateway_info::db::all_info_stream(&pool, &device_types);
if request.min_updated_at > 0 {
let min_updated_at = Utc
.timestamp_opt(request.min_updated_at as i64, 0)
.single()
.ok_or(Status::invalid_argument(
"Invalid min_refreshed_at argument",
))?;

let updated_redios = get_updated_radios(&mc_pool, min_updated_at).await?;
let stream = stream
.filter(|v| future::ready(updated_redios.contains(&v.address)))
.boxed();
stream_multi_gateways_info(stream, tx.clone(), signing_key.clone(), batch_size)
.await
} else {
stream_multi_gateways_info(stream, tx.clone(), signing_key.clone(), batch_size)
.await
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let stream = gateway_info::db::all_info_stream(&pool, &device_types);
if request.min_updated_at > 0 {
let min_updated_at = Utc
.timestamp_opt(request.min_updated_at as i64, 0)
.single()
.ok_or(Status::invalid_argument(
"Invalid min_refreshed_at argument",
))?;
let updated_redios = get_updated_radios(&mc_pool, min_updated_at).await?;
let stream = stream
.filter(|v| future::ready(updated_redios.contains(&v.address)))
.boxed();
stream_multi_gateways_info(stream, tx.clone(), signing_key.clone(), batch_size)
.await
} else {
stream_multi_gateways_info(stream, tx.clone(), signing_key.clone(), batch_size)
.await
}
let stream =
if request.min_updated_at > 0 {
let min_updated_at = Utc
.timestamp_opt(request.min_updated_at as i64, 0)
.single()
.ok_or(Status::invalid_argument(
"Invalid min_refreshed_at argument",
))?;
let updated_redios = get_updated_radios(&mc_pool, min_updated_at).await?;
gateway_info::db::all_info_stream(&pool, &device_types)
.filter(|v| future::ready(updated_redios.contains(&v.address)))
.boxed();
} else {
gateway_info::db::all_info_stream(&pool, &device_types)
};
stream_multi_gateways_info(stream, tx.clone(), signing_key.clone(), batch_size).await

i don't know that this really makes a difference; i wanted to get it down to a single invocation of the stream but i don't think we can fully clean that up unless we added a no-op filter on the branch that doesn't check updated at so feel free to ignore here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to do it in a similar way, but I'm facing incompatible types or borrow checker errors.

@kurotych kurotych changed the title [Mobile config] min_refreshed_at => min_updated_at [Mobile config] Change min_refreshed_at to min_updated_at in gateway_info_v2 Dec 9, 2024
@kurotych kurotych marked this pull request as draft December 10, 2024 17:04
@kurotych
Copy link
Member Author

PR is converted back to draft
helium/proto#434 (comment)

@kurotych kurotych marked this pull request as ready for review December 11, 2024 12:42
@kurotych kurotych changed the title [Mobile config] Change min_refreshed_at to min_updated_at in gateway_info_v2 [Mobile config] Add v2 endpoints to GatewayInfoV2. Change min_refreshed_at to min_updated_at Dec 17, 2024
@kurotych kurotych merged commit 6a8bb9b into main Dec 17, 2024
17 checks passed
@kurotych kurotych deleted the min_update_at branch December 17, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants