Skip to content

fix: value-based equality and hashCode for BleCharacteristic#267

Open
xianjianlf2 wants to merge 1 commit into
Navideck:mainfrom
xianjianlf2:fix/ble-characteristic-equality-list
Open

fix: value-based equality and hashCode for BleCharacteristic#267
xianjianlf2 wants to merge 1 commit into
Navideck:mainfrom
xianjianlf2:fix/ble-characteristic-equality-list

Conversation

@xianjianlf2

Copy link
Copy Markdown
Contributor

Problem

BleCharacteristic.operator== compares the properties list with !=, which is List identity comparison in Dart, and hashCode uses properties.hashCode, which is also identity-based. As a result, two characteristics with identical content are never equal unless they share the exact same list instance:

final a = BleCharacteristic("180A", [CharacteristicProperty.read]);
final b = BleCharacteristic("180A", [CharacteristicProperty.read]);
print(a == b); // false — but they are logically equal

This breaks the ==/hashCode contract and makes BleCharacteristic unreliable as a Set element or Map key (e.g. deduplicating characteristics across discovery runs).

Fix

  • operator== now compares properties with listEquals.
  • hashCode now uses Object.hashAll(properties).

This matches the pattern already used by ManufacturerData in this package.

Testing

  • Added test/ble_characteristic_equality_test.dart covering: equal content ⇒ equal + same hash, differing properties ⇒ unequal, differing uuid ⇒ unequal, and Set/Map key usage.
  • Ran flutter test test/ble_characteristic_equality_test.dart locally (Flutter 3.44.1 / Dart 3.12.1): 4 tests, all passing.

Changelog

Added an entry under the unreleased 2.1.1 section in CHANGELOG.md.

BleCharacteristic.operator== compared the `properties` list with `!=`,
which is List identity comparison, and hashCode used `properties.hashCode`
(also identity based). As a result two characteristics with equal content
but distinct list instances were never equal and produced different hash
codes, breaking the equality contract and Set/Map lookups.

Use `listEquals` for comparison and `Object.hashAll` for hashing, matching
the pattern already used in ManufacturerData.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request fixes the equality and hashCode implementation of BleCharacteristic to compare properties by value using listEquals and Object.hashAll, and adds corresponding unit tests. The reviewer suggested using Object.hash instead of the bitwise XOR (^) operator to combine hash codes to prevent potential hash collisions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +81 to +82
int get hashCode =>
uuid.hashCode ^ Object.hashAll(properties) ^ metaData.hashCode;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using the bitwise XOR (^) operator to combine hash codes is prone to hash collisions (e.g., if fields have identical hash values, they cancel each other out). It is a best practice in Dart to use Object.hash to combine multiple hash codes safely.

Suggested change
int get hashCode =>
uuid.hashCode ^ Object.hashAll(properties) ^ metaData.hashCode;
int get hashCode => Object.hash(
uuid,
Object.hashAll(properties),
metaData,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@xianjianlf2 could you please check this comment?

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.

2 participants