From 6bb0d0cd96869f7217e7c90b77b6dc9d6755780b Mon Sep 17 00:00:00 2001 From: Aaron Kromer Date: Wed, 13 Dec 2017 07:12:45 -0500 Subject: [PATCH 1/4] Use annotations and final fields to express intent Based on the code the implicit expectation is that the internal map will never be `null`. Additionally, the provided `Beacon` to track should also never be `null`. Adding the annotations helps let the Android Studio code inspector and the lint tools know about this. This can help prevent accidental NPE issues if someone can produce a `null` beacon which then gets passed here. While the map may change the variable is never re-assigned. Looking at the code which uses this class when it needs to change the flag the clients simply creates a new instance. Marking the fields `final` makes this behavior intent clear. --- .../service/ExtraDataBeaconTracker.java | 36 ++++++++++++------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java b/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java index 303efdd4d..7d689f09b 100644 --- a/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java +++ b/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java @@ -1,5 +1,8 @@ package org.altbeacon.beacon.service; +import android.support.annotation.NonNull; +import android.support.annotation.Nullable; + import org.altbeacon.beacon.Beacon; import java.io.Serializable; @@ -12,11 +15,17 @@ */ public class ExtraDataBeaconTracker implements Serializable { private static final String TAG = "BeaconTracker"; - // This is a lookup table to find tracked beacons by the calculated beacon key - private HashMap> mBeaconsByKey = new HashMap>(); - private boolean matchBeaconsByServiceUUID = true; + /** + * This is a lookup table to find tracked beacons by the calculated beacon key + */ + @NonNull + private final HashMap> mBeaconsByKey = new HashMap<>(); + + private final boolean matchBeaconsByServiceUUID; + public ExtraDataBeaconTracker() { + this(true); } public ExtraDataBeaconTracker(boolean matchBeaconsByServiceUUID) { @@ -25,12 +34,10 @@ public ExtraDataBeaconTracker(boolean matchBeaconsByServiceUUID) { /** * Tracks a beacon. For Gatt-based beacons, returns a merged copy of fields from multiple - * frames. Returns null when passed a Gatt-based beacon that has is only extra beacon data. - * - * @param beacon - * @return + * frames. Returns null when passed a Gatt-based beacon that has is only extra beacon data. */ - public synchronized Beacon track(Beacon beacon) { + @Nullable + public synchronized Beacon track(@NonNull Beacon beacon) { Beacon trackedBeacon = null; if (beacon.isMultiFrameBeacon() || beacon.getServiceUuid() != -1) { trackedBeacon = trackGattBeacon(beacon); @@ -41,8 +48,11 @@ public synchronized Beacon track(Beacon beacon) { return trackedBeacon; } - // The following code is for dealing with merging data fields in beacons - private Beacon trackGattBeacon(Beacon beacon) { + /** + * The following code is for dealing with merging data fields in beacons + */ + @Nullable + private Beacon trackGattBeacon(@NonNull Beacon beacon) { Beacon trackedBeacon = null; HashMap matchingTrackedBeacons = mBeaconsByKey.get(getBeaconKey(beacon)); if (matchingTrackedBeacons != null) { @@ -68,15 +78,15 @@ private Beacon trackGattBeacon(Beacon beacon) { return trackedBeacon; } - private void updateTrackingHashes(Beacon trackedBeacon, HashMap matchingTrackedBeacons) { + private void updateTrackingHashes(@NonNull Beacon trackedBeacon, @Nullable HashMap matchingTrackedBeacons) { if (matchingTrackedBeacons == null) { - matchingTrackedBeacons = new HashMap(); + matchingTrackedBeacons = new HashMap<>(); } matchingTrackedBeacons.put(trackedBeacon.hashCode(), trackedBeacon); mBeaconsByKey.put(getBeaconKey(trackedBeacon), matchingTrackedBeacons); } - private String getBeaconKey(Beacon beacon) { + private String getBeaconKey(@NonNull Beacon beacon) { if (matchBeaconsByServiceUUID) { return beacon.getBluetoothAddress() + beacon.getServiceUuid(); } else { From a071ca40f90079a58c002ef6aefa30e239eb7e98 Mon Sep 17 00:00:00 2001 From: Aaron Kromer Date: Wed, 13 Dec 2017 10:06:36 -0500 Subject: [PATCH 2/4] Fix up extra data beacon test cases This adjusts the test cases to ensure all of the branches get tested. The `gattBeaconExtraDataGetUpdated` test is updated to show that each GATT data beacon causes the tracked beacon to be updated. This was implicitly assumed previously but this makes it explicit by re-checked after each tracking call. This removes the `gattBeaconExtraDataAreNotOverwritten` test. This test is confusing. It is worded to indicate a change shouldn't happen even though a change does occur to `beacon` / `trackedBeacon`. Since a change occurs to `beacon` / `trackedBeacon` which is used in the assertion we cannot say this proves `extraDataBeacon` wasn't modified. This adds a new test `gattBeaconFieldsAreNotUpdated` to demonstrate that without the GATT data beacons no changes to tracked beacons occur. Conversely, this updates `gattBeaconFieldsGetUpdated` to prove that the non-data beacons are updated based on the stored tracked beacons. It uses a new "repeat" beacon to show that the extra data was modified, but the main data and RSSI were left as reported by that beacon. The repeat beacon is required because the original `beacon` is mutated and thus would incorrectly show changes to the RSSI and data. --- .../service/ExtraDataBeaconTrackerTest.java | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/test/java/org/altbeacon/beacon/service/ExtraDataBeaconTrackerTest.java b/src/test/java/org/altbeacon/beacon/service/ExtraDataBeaconTrackerTest.java index cc7dcd998..24927ac5c 100644 --- a/src/test/java/org/altbeacon/beacon/service/ExtraDataBeaconTrackerTest.java +++ b/src/test/java/org/altbeacon/beacon/service/ExtraDataBeaconTrackerTest.java @@ -10,11 +10,9 @@ import java.util.ArrayList; import java.util.List; -import dalvik.annotation.TestTargetClass; - import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; @RunWith(RobolectricTestRunner.class) @Config(sdk = 18) @@ -59,6 +57,7 @@ Beacon getGattBeaconExtraData() { return new Beacon.Builder() .setBluetoothAddress("01:02:03:04:05:06") .setServiceUuid(1234) + .setRssi(-25) .setDataFields(getDataFields()) .build(); } @@ -67,6 +66,7 @@ Beacon getGattBeaconExtraData2() { return new Beacon.Builder() .setBluetoothAddress("01:02:03:04:05:06") .setServiceUuid(1234) + .setRssi(-50) .setDataFields(getDataFields2()) .build(); } @@ -119,32 +119,44 @@ public void gattBeaconExtraDataGetUpdated() { ExtraDataBeaconTracker tracker = new ExtraDataBeaconTracker(); tracker.track(beacon); tracker.track(extraDataBeacon); - tracker.track(extraDataBeacon2); Beacon trackedBeacon = tracker.track(beacon); + assertEquals("rssi should be updated", extraDataBeacon.getRssi(), trackedBeacon.getRssi()); + assertEquals("extra data is updated", extraDataBeacon.getDataFields(), trackedBeacon.getExtraDataFields()); + + tracker.track(extraDataBeacon2); + trackedBeacon = tracker.track(beacon); + assertEquals("rssi should be updated", extraDataBeacon2.getRssi(), trackedBeacon.getRssi()); assertEquals("extra data is updated", extraDataBeacon2.getDataFields(), trackedBeacon.getExtraDataFields()); } @Test - public void gattBeaconExtraDataAreNotOverwritten() { + public void gattBeaconFieldsAreNotUpdated() { Beacon beacon = getGattBeacon(); - Beacon extraDataBeacon = getGattBeaconExtraData(); + final int originalRssi = beacon.getRssi(); + final List originalData = beacon.getDataFields(); + final List originalExtra = beacon.getExtraDataFields(); + Beacon beaconUpdate = getGattBeaconUpdate(); ExtraDataBeaconTracker tracker = new ExtraDataBeaconTracker(); tracker.track(beacon); - tracker.track(extraDataBeacon); + tracker.track(beaconUpdate); Beacon trackedBeacon = tracker.track(beacon); - assertEquals("extra data should not be overwritten", extraDataBeacon.getDataFields(), trackedBeacon.getExtraDataFields()); + assertEquals("rssi should NOT be updated", originalRssi, trackedBeacon.getRssi()); + assertEquals("data should NOT be updated", originalData, trackedBeacon.getDataFields()); + assertEquals("extra data should NOT be updated", originalExtra, trackedBeacon.getExtraDataFields()); } @Test public void gattBeaconFieldsGetUpdated() { Beacon beacon = getGattBeacon(); - Beacon beaconUpdate = getGattBeaconUpdate(); Beacon extraDataBeacon = getGattBeaconExtraData(); + Beacon repeatBeacon = getGattBeacon(); + repeatBeacon.setRssi(-100); ExtraDataBeaconTracker tracker = new ExtraDataBeaconTracker(); tracker.track(beacon); - Beacon trackedBeacon = tracker.track(beaconUpdate); - assertEquals("rssi should be updated", beaconUpdate.getRssi(), trackedBeacon.getRssi()); - assertEquals("data fields should be updated", beaconUpdate.getDataFields(), trackedBeacon.getDataFields()); + tracker.track(extraDataBeacon); + Beacon trackedBeacon = tracker.track(repeatBeacon); + assertEquals("rssi should NOT be updated", -100, trackedBeacon.getRssi()); + assertEquals("extra data fields should be updated", extraDataBeacon.getDataFields(), trackedBeacon.getExtraDataFields()); } @Test From 83f6dc2e509a87f11f22c6d8640702e4040a11f5 Mon Sep 17 00:00:00 2001 From: Aaron Kromer Date: Wed, 13 Dec 2017 10:26:16 -0500 Subject: [PATCH 3/4] Minor refactor to improve separate update branches This extracts the GATT data beacon path into a guard clause. This helps to highlight the fact that these beacons: - are not modified - update any existing tracked beacons - return `null` This also has the benefit or removing the conditional recheck within the loop. This also makes it clear that we update the extra data based on any one of the tracked beacons (as they all have the same data); instead of resetting within the loop. The tracking hash update logic is now inline within the main `trackGattBeacon` method. It was only called from this method and only for non-GATT data beacons. Also, since the GATT data logic is split out we still need to set the extra data. Accessing the tracked beacons once here keeps all of this logic together making it easier to see what is happening on an non-data beacon update. --- .../service/ExtraDataBeaconTracker.java | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java b/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java index 7d689f09b..ae1bbbb19 100644 --- a/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java +++ b/src/main/java/org/altbeacon/beacon/service/ExtraDataBeaconTracker.java @@ -53,37 +53,34 @@ public synchronized Beacon track(@NonNull Beacon beacon) { */ @Nullable private Beacon trackGattBeacon(@NonNull Beacon beacon) { - Beacon trackedBeacon = null; - HashMap matchingTrackedBeacons = mBeaconsByKey.get(getBeaconKey(beacon)); - if (matchingTrackedBeacons != null) { - for (Beacon matchingTrackedBeacon: matchingTrackedBeacons.values()) { - if (beacon.isExtraBeaconData()) { - matchingTrackedBeacon.setRssi(beacon.getRssi()); - matchingTrackedBeacon.setExtraDataFields(beacon.getDataFields()); - } - else { - beacon.setExtraDataFields(matchingTrackedBeacon.getExtraDataFields()); - // replace the tracked beacon instance with this one so it has updated values - trackedBeacon = beacon; - } - } - } - if (!beacon.isExtraBeaconData()) { - updateTrackingHashes(beacon, matchingTrackedBeacons); + if (beacon.isExtraBeaconData()) { + updateTrackedBeacons(beacon); + return null; } - if (trackedBeacon == null && !beacon.isExtraBeaconData()) { - trackedBeacon = beacon; + String key = getBeaconKey(beacon); + HashMap matchingTrackedBeacons = mBeaconsByKey.get(key); + if (null == matchingTrackedBeacons) { + matchingTrackedBeacons = new HashMap<>(); } - return trackedBeacon; + else { + Beacon trackedBeacon = matchingTrackedBeacons.values().iterator().next(); + beacon.setExtraDataFields(trackedBeacon.getExtraDataFields()); + } + matchingTrackedBeacons.put(beacon.hashCode(), beacon); + mBeaconsByKey.put(key, matchingTrackedBeacons); + + return beacon; } - private void updateTrackingHashes(@NonNull Beacon trackedBeacon, @Nullable HashMap matchingTrackedBeacons) { - if (matchingTrackedBeacons == null) { - matchingTrackedBeacons = new HashMap<>(); + private void updateTrackedBeacons(@NonNull Beacon beacon) { + HashMap matchingTrackedBeacons = mBeaconsByKey.get(getBeaconKey(beacon)); + if (null != matchingTrackedBeacons) { + for (Beacon matchingTrackedBeacon : matchingTrackedBeacons.values()) { + matchingTrackedBeacon.setRssi(beacon.getRssi()); + matchingTrackedBeacon.setExtraDataFields(beacon.getDataFields()); + } } - matchingTrackedBeacons.put(trackedBeacon.hashCode(), trackedBeacon); - mBeaconsByKey.put(getBeaconKey(trackedBeacon), matchingTrackedBeacons); } private String getBeaconKey(@NonNull Beacon beacon) { From da5ce17700f1e9d534cf23dfc03a80313abbe84e Mon Sep 17 00:00:00 2001 From: Aaron Kromer Date: Wed, 13 Dec 2017 10:39:56 -0500 Subject: [PATCH 4/4] Help protected against NPE for extra data tracking This is a follow up to #616, #617, and #626. It uses annotations to indicate that we never expect this field to be `null`. It also sets a default tracker on initialization. This ensures there is always at tracker available; even if it's only temporary. The work in the other PRs makes sure the tracker is re-initialized at the appropriate times with the correct settings. --- src/main/java/org/altbeacon/beacon/service/ScanHelper.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/altbeacon/beacon/service/ScanHelper.java b/src/main/java/org/altbeacon/beacon/service/ScanHelper.java index d44700ccd..8821006ef 100644 --- a/src/main/java/org/altbeacon/beacon/service/ScanHelper.java +++ b/src/main/java/org/altbeacon/beacon/service/ScanHelper.java @@ -65,7 +65,10 @@ class ScanHelper { private MonitoringStatus mMonitoringStatus; private final Map mRangedRegionState = new HashMap<>(); private DistinctPacketDetector mDistinctPacketDetector = new DistinctPacketDetector(); - private ExtraDataBeaconTracker mExtraDataBeaconTracker; + + @NonNull + private ExtraDataBeaconTracker mExtraDataBeaconTracker = new ExtraDataBeaconTracker(); + private Set mBeaconParsers = new HashSet<>(); private List mSimulatedScanData = null; private Context mContext; @@ -99,7 +102,7 @@ void setRangedRegionState(Map rangedRegionState) { } } - void setExtraDataBeaconTracker(ExtraDataBeaconTracker extraDataBeaconTracker) { + void setExtraDataBeaconTracker(@NonNull ExtraDataBeaconTracker extraDataBeaconTracker) { mExtraDataBeaconTracker = extraDataBeaconTracker; }