-
Notifications
You must be signed in to change notification settings - Fork 35
[mobile_config] track owner refactoring #1131
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Refactors the mobile gateway tracking/data model to consolidate hash-related fields (including owner) under a new HashParams container, updates integration tests accordingly, and adds a temporary runtime backfill to repair missing hashes during a migration.
Changes:
- Move gateway “hash inputs” (type/location/deployment/owner/etc.) into
HashParamsand update call sites to use accessor methods. - Add a temporary
backfill_hashespath in the gateway tracker to populate missing hashes. - Add a migration that nulls out
gateways.hashand update integration tests to reflect the refactor (e.g., removedinserted_atin test builders/usages).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| mobile_config/src/gateway/db.rs | Introduces HashParams, refactors Gateway fields/accessors, adds null-hash backfill helpers. |
| mobile_config/src/gateway/tracker.rs | Adds backfill_hashes and refactors execute to use new Gateway construction/change detection. |
| mobile_config/src/gateway/metadata_db.rs | Refactors MobileHotspotInfo to expose fields needed by tracker/db and filters CBRS in SQL. |
| mobile_config/src/gateway/service/info.rs | Updates gateway info conversion to use new accessors. |
| mobile_config/src/gateway/service/info_v3.rs | Updates v3 conversion to use new accessors. |
| mobile_config/src/gateway/service/info_v4.rs | Updates v4 conversion to use new accessors and owner sourcing. |
| mobile_config/migrations/20260209112231_null_hash.sql | Adds migration that sets gateways.hash to NULL (intended to trigger backfill). |
| mobile_config/tests/integrations/*.rs | Updates integration tests to use new accessors and HashParams construction; removes inserted_at usage in builders. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| backfill_hashes(&self.pool, &self.metadata).await?; | ||
| loop { |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backfill_hashes introduces migration-critical behavior (repairing NULL hashes) but there are no integration/unit tests covering it (existing integration tests call tracker::execute directly and won’t exercise the new backfill path). Adding a focused test that sets gateways.hash to NULL and asserts backfill_hashes updates the latest row’s hash (and does not regress execute) would help prevent a production-only failure during/after the migration.
| @@ -0,0 +1 @@ | |||
| UPDATE gateways SET hash = NULL; | |||
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new 20260209112231_null_hash.sql sets gateways.hash to NULL for every row. Since Gateway::from_row and several queries (e.g. get_by_address, get_by_addresses, streams) still select hash directly and deserialize it into a non-optional String, this migration can cause runtime decode errors anywhere gateways are read before the tracker backfill runs (or for addresses not covered by the metadata stream). Consider backfilling hashes in the migration (or at least setting to empty string), or making reads tolerate NULL (e.g., COALESCE in all selects / Option<String> during migration) until the column is repopulated.
| created_at: row.try_get("created_at")?, | ||
| inserted_at: row.try_get("inserted_at")?, | ||
| last_changed_at: row.try_get("last_changed_at")?, | ||
| hash: row.try_get("hash")?, | ||
| antenna: to_u32(row.try_get("antenna")?), | ||
| elevation: to_u32(row.try_get("elevation")?), | ||
| azimuth: to_u32(row.try_get("azimuth")?), | ||
| location: to_u64(row.try_get("location")?), | ||
| location_changed_at: row.try_get("location_changed_at")?, | ||
| location_asserts: to_u32(row.try_get("location_asserts")?), | ||
| owner: row.try_get("owner")?, | ||
| owner_changed_at: row.try_get("owner_changed_at")?, |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gateway::from_row deserializes hash as a non-optional String (row.try_get("hash")?). Combined with the new migration that sets hash = NULL, any query that selects hash without COALESCE(hash, '') will fail to decode until backfilled. To make the migration safe, either change hash to Option<String> temporarily, or update all gateway SELECTs to COALESCE hash, or avoid setting existing rows to NULL in the first place.
| let count: i64 = sqlx::query_scalar( | ||
| r#" | ||
| SELECT COUNT(*) FROM ( | ||
| SELECT DISTINCT ON (address) | ||
| address, | ||
| gateway_type, | ||
| created_at, | ||
| last_changed_at, | ||
| COALESCE(hash, '') as hash, | ||
| antenna, | ||
| elevation, | ||
| azimuth, | ||
| location, | ||
| location_changed_at, | ||
| location_asserts, | ||
| owner, | ||
| owner_changed_at | ||
| FROM gateways | ||
| ORDER BY address, inserted_at DESC | ||
| ) AS distinct_gateways | ||
| WHERE hash = '' | ||
| "#, | ||
| ) |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backfill_hashes counts “null hash” gateways via COALESCE(hash, '') as hash ... WHERE hash = '', but the actual fetch/update path uses Gateway::get_by_addresses_with_null_hash which filters strictly on hash IS NULL. This can lead to count > 0 while total updated is 0 (e.g., when hash is an empty string rather than NULL) and makes the log message misleading. Align both sides to the same definition (prefer hash IS NULL everywhere, or handle both NULL and empty).
| .unwrap_or([0u8; 4]) | ||
| .as_ref(), | ||
| ); | ||
| hasher.update(self.owner.as_deref().unwrap_or_default().as_bytes()); |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashParams::compute_hash now includes owner (hasher.update(self.owner... )). In Gateway::new_if_changed, owner_changed intentionally ignores transitions where the new owner is None, but because the hash includes owner, a Some(x) -> None change will still change new_hash and create a new Gateway row while leaving owner_changed_at unchanged. This makes owner_changed_at inconsistent with the stored owner and can break consumers that rely on owner_changed_at for change tracking. Consider either excluding owner from the hash, or normalizing None to “keep last owner” for hash purposes, or updating the owner_changed/owner_changed_at logic to treat Some -> None as a change if you want owner removals to be tracked.
| hasher.update(self.owner.as_deref().unwrap_or_default().as_bytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transferring from Some(Owner) to None is not expected. Ignoring such "change" in mobile_hotspot_info table is intentional
| owner_changed_at | ||
| FROM gateways | ||
| WHERE address = ANY($1) | ||
| AND hash IS NULL |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gateway::get_by_addresses_with_null_hash filters with AND hash IS NULL but also selects COALESCE(hash, '') as hash. If the intent is to backfill both NULL and empty-string hashes (as backfill_hashes’s COUNT query suggests), this method won’t return rows where hash = ''. Either broaden the predicate to include empty strings too, or adjust the upstream COUNT/logging to match hash IS NULL only.
| AND hash IS NULL | |
| AND (hash IS NULL OR hash = '') |
The main change is creating a new hash calculation based on the
Gatewayfields instead of counterintuitive hashing inmobile_hotspot_infoand then saving it into the gateways table.The additional reason for creating a new hash mechanism is using the
ownerfield for tracking changes that is not part of themobile_hotspot_infotable. Now ifownerchanged thenhashis recalculated.Additional improvements:
executefunctionP.S.
The migration was added that set all hash to NULL in gateways table,
during startup the backfill_hashes function will be executed the recalculates hashes (locally it takes less then 1 min)