[mobile config] track asset ownership (part 1) #1102

kurotych · 2025-12-09T17:44:16Z

This is stage 1 of starting to track the owner (with owner_changed_at) field.

owner and owner_changed_at fields were added to the gateways table
Backfill function (that fills all latest gateways records with owner) was added and run before tracking.
If owner is changed, then a new record in the gateways table is produced but only owner and owner_changed_at are updated. The last_changed_at is not updated at this stage, since I don't want to affect API behavior for now.

P.S. The backfill function (locally) with prod db takes around 30 seconds. (second start takes nothing sine early exit works

    if null_owner_addresses.is_empty() {
        return Ok(());
    }

On the second stage after we sure everything is ok in prod I'm gonna add new fields to grpc response

Copilot

Pull request overview

This PR implements asset ownership tracking for mobile gateways by adding owner and owner_changed_at fields to the gateway system. The implementation includes database schema changes, backfill functionality for existing gateways, and updates to the tracker logic to detect and record owner changes.

Key Changes:

Added owner and owner_changed_at fields to the Gateway model and database
Implemented backfill functionality to populate owner data for existing gateways with NULL owners
Updated gateway tracker to detect owner changes and create new records when ownership changes
Removed unused import CLI functionality

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
mobile_config/migrations/20251215132218_add_owner_hash_to_gateways.sql	Adds owner and owner_changed_at columns to gateways table
mobile_config/migrations/20251209173943_drop_mobile_radio_tracker.sql	Removes obsolete mobile_radio_tracker table
mobile_config/src/gateway/db.rs	Adds owner fields to Gateway struct and implements get_null_owners and update_owners methods
mobile_config/src/gateway/metadata_db.rs	Adds owner field to MobileHotspotInfo with LEFT JOIN to asset_owners table
mobile_config/src/gateway/tracker.rs	Implements backfill_gateway_owners function and updates execute to track owner changes
mobile_config/tests/integrations/gateway_tracker.rs	Adds comprehensive tests for owner tracking and backfill functionality
mobile_config/tests/integrations/common/gateway_db.rs	Introduces TestGatewayBuilder for simplified test gateway construction
mobile_config/tests/integrations/common/gateway_metadata_db.rs	Adds helper functions and table creation for asset_owners
mobile_config/tests/integrations/gateway_service*.rs	Updates tests to use TestGatewayBuilder and adds owner fields to manual Gateway construction
mobile_config/src/cli/mod.rs	Removes unused Import CLI command
mobile_config/src/cli/import.rs	Deleted - removes unused import functionality
mobile_config/Cargo.toml	Adds derive_builder as dev dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T14:19:26Z

mobile_config/src/gateway/tracker.rs

+        if let Err(e) = backfill_gateway_owners(&self.pool, &self.metadata).await {
+            tracing::error!("backfill_gateway_owners is faled. {e}");
+        }


The backfill function runs on every service start which could cause performance issues if there are many gateways. Consider adding a check to run this only once (e.g., using a migration flag or a separate one-time command) or adding a mechanism to prevent it from running repeatedly after the initial backfill is complete.

Copilot · 2025-12-17T14:19:27Z

mobile_config/tests/integrations/gateway_service_v3.rs

+    let gateway1: Gateway = TestGatewayBuilder::default()
+        .address(address1.clone())
+        .gateway_type(GatewayType::WifiIndoor)
+        .created_at(now)
+        .inserted_at(now)
+        .refreshed_at(now)
+        .last_changed_at(now_plus_10)
+        .elevation(2)
+        .azimuth(161)
+        .location(loc1)
+        .location_changed_at(now_plus_5)
+        .location_asserts(1)
+        .build()?
+        .into();


The test expects antenna to be 18 (line 85), but the TestGatewayBuilder for gateway1 doesn't explicitly set the antenna field. Looking at the builder definition, the default antenna value is Some(18), so this works. However, this implicit dependency on the default value makes the test fragile. Consider explicitly setting .antenna(18) in the builder to make the test's expectations clear and prevent future breakage if the default changes.

Copilot · 2025-12-17T14:19:27Z

mobile_config/migrations/20251215132218_add_owner_to_gateways.sql

@@ -0,0 +1,3 @@
+ALTER TABLE gateways
+ADD COLUMN IF NOT EXISTS owner VARCHAR(255),


The migration file name includes "hash" (add_owner_hash_to_gateways) but the migration only adds owner and owner_changed_at columns, not anything hash-related. The file name should be updated to accurately reflect what the migration does, such as "add_owner_to_gateways.sql".

Suggested change

ADD COLUMN IF NOT EXISTS owner VARCHAR(255),

ADD COLUMN IF NOT EXISTS owner VARCHAR(255),

ADD COLUMN IF NOT EXISTS owner_hash VARCHAR(255),

Copilot · 2025-12-17T14:19:27Z

mobile_config/src/gateway/tracker.rs

+                        let owner_changed = if gw.owner.is_none() {
+                            false
+                        } else {
+                            gw.owner != last_gw.owner
+                        };


The owner change detection logic treats the case where the new owner is None as "no change" (lines 178-182). However, if a gateway previously had an owner and it changes to None, this should be detected as an owner change. Consider updating the logic to handle the case where the owner changes from Some(value) to None.

Suggested change

let owner_changed = if gw.owner.is_none() {

false

} else {

gw.owner != last_gw.owner

};

let owner_changed = gw.owner != last_gw.owner;

Copilot · 2025-12-17T14:19:28Z

mobile_config/tests/integrations/common/gateway_metadata_db.rs

+    sqlx::query(
+        r#"
+        CREATE TABLE asset_owners (
+            asset character varying(255) NULL,
+            owner character varying(255) NULL,
+            created_at timestamptz,
+            updated_at timestamptz,
+            last_block integer
+        );"#,


The asset_owners test table creation doesn't include any indexes on the asset column, but the queries in insert_asset_owner and update_asset_owner filter by asset. While this is acceptable for testing with small datasets, consider adding a comment noting that the production database should have proper indexes on the asset column for performance.

Copilot · 2025-12-17T14:19:28Z

mobile_config/src/gateway/tracker.rs

        let mut interval = tokio::time::interval(self.interval);

+        if let Err(e) = backfill_gateway_owners(&self.pool, &self.metadata).await {
+            tracing::error!("backfill_gateway_owners is faled. {e}");


Spelling error: "faled" should be "failed".

Suggested change

tracing::error!("backfill_gateway_owners is faled. {e}");

tracing::error!("backfill_gateway_owners is failed. {e}");

Copilot · 2025-12-17T14:19:28Z

mobile_config/src/gateway/metadata_db.rs

            },
            location_asserts: self.num_location_asserts.map(|n| n as u32),
+            owner: self.owner.clone(),
+            owner_changed_at: Some(refreshed_at),


The owner_changed_at is being set in the to_gateway() conversion, but this should be set based on when the owner actually changed, not always to refreshed_at. This could lead to incorrect owner_changed_at timestamps on initial creation or when the owner hasn't actually changed.

Suggested change

owner_changed_at: Some(refreshed_at),

// Do not infer owner_changed_at from refreshed_at; this should be set where owner changes are tracked

owner_changed_at: None,

Copilot · 2025-12-17T14:19:29Z

mobile_config/tests/integrations/gateway_tracker.rs

+    // last_changed_at should also be updated since owner changed (In next owner implementing stage).
+    // But currently, it stays the same. After full implementation change `now` to `update_time`


The comment mentions "In next owner implementing stage" which suggests incomplete implementation. However, looking at the tracker.rs code (lines 184-185), there's a TODO comment that aligns with this. The test expects last_changed_at to remain at now, but this may not reflect the final desired behavior where owner changes should also update last_changed_at. Consider updating this test expectation to match the final intended behavior or removing the comment if this is the expected behavior.

Suggested change

// last_changed_at should also be updated since owner changed (In next owner implementing stage).

// But currently, it stays the same. After full implementation change `now` to `update_time`

// Verify that last_changed_at was not updated when the owner changed.

// This test asserts the current behavior where last_changed_at remains equal to `now`.

kurotych added 4 commits December 9, 2025 19:42

Add migration: drop_mobile_radio_tracker.sql

ee62432

Merge branch 'main' into kurotych/mobile_config/track_asset_ownership

4799030

Add add_owner_hash_to_gateways migration

87d80bb

Update db func

1e4d9ae

kurotych force-pushed the kurotych/mobile_config/track_asset_ownership branch from a3f3591 to 0a9724e Compare December 15, 2025 14:13

Fix and refactor tests

397eafd

kurotych force-pushed the kurotych/mobile_config/track_asset_ownership branch from 0a9724e to 397eafd Compare December 15, 2025 14:17

kurotych added 15 commits December 15, 2025 16:24

Merge branch 'main' into kurotych/mobile_config/track_asset_ownership

37d5e17

Use gateway builder in tests

746b95e

Refactor updating gateways logic

e35bc33

Merge branch 'main' into kurotych/mobile_config/track_asset_ownership

4c97859

Add owner_chagned_at field

336e54c

Track owner changing correctly

2b843fa

Delete mobile_config cli import

24ee7e3

Track owner

1d88a37

Merge branch 'main' into kurotych/mobile_config/track_asset_ownership

c92432a

Backfill owners

7f8cdb1

Add test_gateway_tracker_owner_tracking

62a88fc

Make sure new record was created

5475cf2

Don't updated (yet) last_changed_at if owner was changed

c9ec56f

Add backfill owner test

147a132

Fix fmt

e8d76d3

kurotych requested a review from Copilot December 17, 2025 14:12

Copilot started reviewing on behalf of kurotych December 17, 2025 14:12 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

Rename migration file

e12e063

kurotych changed the title ~~[mobile config] track asset ownership~~ [mobile config] track asset ownership (part 1) Dec 17, 2025

kurotych requested review from bbalser, macpie and michaeldjeffrey December 17, 2025 14:38

kurotych requested review from connormck333 and jeffgrunewald December 17, 2025 14:38

kurotych marked this pull request as ready for review December 17, 2025 14:42

connormck333 approved these changes Dec 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mobile config] track asset ownership (part 1) #1102

[mobile config] track asset ownership (part 1) #1102

Uh oh!

kurotych commented Dec 9, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,3 @@
		ALTER TABLE gateways
		ADD COLUMN IF NOT EXISTS owner VARCHAR(255),

	ADD COLUMN IF NOT EXISTS owner VARCHAR(255),
	ADD COLUMN IF NOT EXISTS owner VARCHAR(255),
	ADD COLUMN IF NOT EXISTS owner_hash VARCHAR(255),

	tracing::error!("backfill_gateway_owners is faled. {e}");
	tracing::error!("backfill_gateway_owners is failed. {e}");

	owner_changed_at: Some(refreshed_at),
	// Do not infer owner_changed_at from refreshed_at; this should be set where owner changes are tracked
	owner_changed_at: None,

		// last_changed_at should also be updated since owner changed (In next owner implementing stage).
		// But currently, it stays the same. After full implementation change `now` to `update_time`

[mobile config] track asset ownership (part 1) #1102

Are you sure you want to change the base?

[mobile config] track asset ownership (part 1) #1102

Uh oh!

Conversation

kurotych commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kurotych commented Dec 9, 2025 •

edited

Loading