Fix: Preserve CSV-imported device data during sync#3295
Conversation
… sync Co-authored-by: kbeaugrand <9513635+kbeaugrand@users.noreply.github.com>
Co-authored-by: kbeaugrand <9513635+kbeaugrand@users.noreply.github.com>
Co-authored-by: kbeaugrand <9513635+kbeaugrand@users.noreply.github.com>
|
@copilot review this PR |
Code Review SummaryThis PR successfully addresses the bug where CSV-imported device data was being overwritten during synchronization. The solution is sound and well-tested. ✅ Strengths
📝 Code Quality ObservationsRepetitive Code Pattern: Lines 149-238 use repetitive conditional checks (~90 lines). While functional, this could be refactored into a helper method: private void UpdatePropertyIfExists<T>(Twin twin, string propertyName,
Func<LorawanDevice, T> getter, Action<LorawanDevice, T> setter, bool isReported = false)
{
var properties = isReported ? twin.Properties.Reported : twin.Properties.Desired;
if (properties.Contains(propertyName))
setter(lorawanDeviceEntity, getter(lorawanDevice));
}However, this refactoring is optional - the current explicit approach is clear and maintainable. ✅ TestingTest
RecommendationApprove - The fix correctly solves the stated problem. The repetitive code is acceptable for clarity and the explicit nature makes debugging easier. No changes required. |
Co-authored-by: kbeaugrand <9513635+kbeaugrand@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where CSV-imported LoRaWAN device properties were being overwritten during periodic IoT Hub synchronization. The previous implementation used AutoMapper's blind mapper.Map() which overwrote all entity properties, including those never sent to IoT Hub.
Changes:
- Modified SyncDevicesJob to use selective property updates instead of unconditional mapping
- Added test coverage for the preservation of database values when properties are absent from Twin
- Added an unrelated bash script for spec-driven development workflow
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/IoTHub.Portal.Infrastructure/Jobs/SyncDevicesJob.cs | Replaced AutoMapper blind mapping with selective property updates based on Twin property presence |
| src/IoTHub.Portal.Tests.Unit/Infrastructure/Jobs/SyncDevicesJobTests.cs | Added test to verify database values are preserved when properties are absent from Twin |
| .specify/scripts/bash/check-prerequisites.sh | Added workflow script for spec-driven development (unrelated to the bug fix) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update only properties that are present in the Twin | ||
| // Base device properties (always updated from Twin) | ||
| lorawanDeviceEntity.Name = lorawanDevice.Name; | ||
| lorawanDeviceEntity.DeviceModelId = lorawanDevice.DeviceModelId; | ||
| lorawanDeviceEntity.Version = lorawanDevice.Version; | ||
| lorawanDeviceEntity.IsConnected = lorawanDevice.IsConnected; | ||
| lorawanDeviceEntity.IsEnabled = lorawanDevice.IsEnabled; | ||
| lorawanDeviceEntity.StatusUpdatedTime = lorawanDevice.StatusUpdatedTime; | ||
| lorawanDeviceEntity.LastActivityTime = lorawanDevice.LastActivityTime; | ||
| lorawanDeviceEntity.LayerId = lorawanDevice.LayerId; | ||
| lorawanDeviceEntity.Tags = lorawanDevice.Tags; | ||
|
|
||
| // Update LoRaWAN properties only if they exist in the Twin's desired properties | ||
| // OTAA/ABP authentication settings | ||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.AppEUI))) | ||
| lorawanDeviceEntity.AppEUI = lorawanDevice.AppEUI; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.AppKey))) | ||
| lorawanDeviceEntity.AppKey = lorawanDevice.AppKey; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.AppSKey))) | ||
| lorawanDeviceEntity.AppSKey = lorawanDevice.AppSKey; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.NwkSKey))) | ||
| lorawanDeviceEntity.NwkSKey = lorawanDevice.NwkSKey; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.DevAddr))) | ||
| lorawanDeviceEntity.DevAddr = lorawanDevice.DevAddr; | ||
|
|
||
| // Update UseOTAA based on AppEUI presence in Twin | ||
| // Only update if AppEUI exists in Twin to avoid overwriting database value | ||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.AppEUI))) | ||
| lorawanDeviceEntity.UseOTAA = lorawanDevice.UseOTAA; | ||
|
|
||
| // Other LoRaWAN configuration properties (only update if present in Twin) | ||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.SensorDecoder))) | ||
| lorawanDeviceEntity.SensorDecoder = lorawanDevice.SensorDecoder; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.ClassType))) | ||
| lorawanDeviceEntity.ClassType = lorawanDevice.ClassType; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.PreferredWindow))) | ||
| lorawanDeviceEntity.PreferredWindow = lorawanDevice.PreferredWindow; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.Deduplication))) | ||
| lorawanDeviceEntity.Deduplication = lorawanDevice.Deduplication; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.RX1DROffset))) | ||
| lorawanDeviceEntity.RX1DROffset = lorawanDevice.RX1DROffset; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.RX2DataRate))) | ||
| lorawanDeviceEntity.RX2DataRate = lorawanDevice.RX2DataRate; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.RXDelay))) | ||
| lorawanDeviceEntity.RXDelay = lorawanDevice.RXDelay; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.ABPRelaxMode))) | ||
| lorawanDeviceEntity.ABPRelaxMode = lorawanDevice.ABPRelaxMode; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.FCntUpStart))) | ||
| lorawanDeviceEntity.FCntUpStart = lorawanDevice.FCntUpStart; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.FCntDownStart))) | ||
| lorawanDeviceEntity.FCntDownStart = lorawanDevice.FCntDownStart; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.FCntResetCounter))) | ||
| lorawanDeviceEntity.FCntResetCounter = lorawanDevice.FCntResetCounter; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.Supports32BitFCnt))) | ||
| lorawanDeviceEntity.Supports32BitFCnt = lorawanDevice.Supports32BitFCnt; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.KeepAliveTimeout))) | ||
| lorawanDeviceEntity.KeepAliveTimeout = lorawanDevice.KeepAliveTimeout; | ||
|
|
||
| if (twin.Properties.Desired.Contains(nameof(LoRaDeviceDetails.Downlink))) | ||
| lorawanDeviceEntity.Downlink = lorawanDevice.Downlink; | ||
|
|
||
| // Update reported properties only if they exist in Twin (as they come from the device) | ||
| // AlreadyLoggedInOnce is set based on DevAddr presence in reported properties | ||
| if (twin.Properties.Reported.Contains("DevAddr")) | ||
| lorawanDeviceEntity.AlreadyLoggedInOnce = lorawanDevice.AlreadyLoggedInOnce; | ||
|
|
||
| if (twin.Properties.Reported.Contains(nameof(LoRaDeviceDetails.GatewayID))) | ||
| lorawanDeviceEntity.GatewayID = lorawanDevice.GatewayID; | ||
|
|
||
| if (twin.Properties.Reported.Contains(nameof(LoRaDeviceDetails.DataRate))) | ||
| lorawanDeviceEntity.DataRate = lorawanDevice.DataRate; | ||
|
|
||
| if (twin.Properties.Reported.Contains(nameof(LoRaDeviceDetails.TxPower))) | ||
| lorawanDeviceEntity.TxPower = lorawanDevice.TxPower; | ||
|
|
||
| if (twin.Properties.Reported.Contains(nameof(LoRaDeviceDetails.NbRep))) | ||
| lorawanDeviceEntity.NbRep = lorawanDevice.NbRep; | ||
|
|
||
| if (twin.Properties.Reported.Contains(nameof(LoRaDeviceDetails.ReportedRX2DataRate))) | ||
| lorawanDeviceEntity.ReportedRX2DataRate = lorawanDevice.ReportedRX2DataRate; | ||
|
|
||
| if (twin.Properties.Reported.Contains(nameof(LoRaDeviceDetails.ReportedRX1DROffset))) | ||
| lorawanDeviceEntity.ReportedRX1DROffset = lorawanDevice.ReportedRX1DROffset; | ||
|
|
||
| if (twin.Properties.Reported.Contains(nameof(LoRaDeviceDetails.ReportedRXDelay))) | ||
| lorawanDeviceEntity.ReportedRXDelay = lorawanDevice.ReportedRXDelay; |
There was a problem hiding this comment.
The mapper's Twin->LorawanDevice mapping uses helper methods like GetDesiredPropertyAsIntegerValue and GetDesiredPropertyAsEnum that return nullable values. When these properties are not present in the Twin, the mapper would set them to null or default values. However, the new implementation only updates properties if they're present in the Twin. This means that if a property WAS in the Twin before but is now removed, it won't be cleared from the database - it will keep the old value. Consider whether properties should be explicitly set to null/default when they're removed from the Twin, or document this as expected behavior.
| d.RX1DROffset == 5 && | ||
| d.KeepAliveTimeout == 120 && | ||
| d.GatewayID == "ExistingGatewayID" && | ||
| d.AlreadyLoggedInOnce == true // Preserved because DevAddr not in reported properties |
There was a problem hiding this comment.
The expression 'A == true' can be simplified to 'A'.
| // Verify Twin properties are updated | ||
| d.AppKey == "NewAppKey" && | ||
| d.AppEUI == "NewAppEUI" && | ||
| d.UseOTAA == true && // Updated because AppEUI is in Twin |
There was a problem hiding this comment.
The expression 'A == true' can be simplified to 'A'.
Description
CSV-imported LoRaWAN device properties were overwritten during periodic IoT Hub synchronization. The sync job used AutoMapper's blind
mapper.Map(), which overwrote all entity properties—including those never sent to IoT Hub (e.g., SensorDecoder, ClassType, Deduplication, RX1DROffset, KeepAliveTimeout).Changes:
Twin.Properties.DesiredTwin.Properties.ReportedExample:
Test coverage: Added
Execute_ExistingLorawanDeviceWithMissingTwinProperties_DatabaseValuesPreservedvalidating preservation of database values when properties absent from Twin.What kind of change does this PR introduce?
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.