Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2280341945
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +310,77 @@ public boolean equals(Object o) {
return false;
}
DefaultServiceInstance that = (DefaultServiceInstance) o;
- boolean equals = Objects.equals(getServiceName(),
that.getServiceName())
- && Objects.equals(getHost(), that.getHost())
- && Objects.equals(getPort(), that.getPort());
- for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
- continue;
+ if (!Objects.equals(serviceName, that.serviceName)
+ || !Objects.equals(host, that.host)
+ || !Objects.equals(port, that.port)) {
+ return false;
+ }
+
+ // compare metadata.
+ return compareMetadata(that.metadata);
+ }
+
+ /**
+ * Compares this instance's metadata with another {@code SortedMap<String,
String>} for equality.
+ * <p>
+ * The comparison is performed in key order. For each key:
+ * <ul>
+ * <li>If the key is EXPORTED_SERVICES_REVISION_PROPERTY_NAME or
TIMESTAMP_KEY,
+ * only the presence and order of the key are checked; their values
are ignored.</li>
+ * <li>For all other keys, both the key and its value must match.</li>
+ * </ul>
+ * <p>
+ * This method is designed to ensure consistency with the {@code hashCode}
implementation,
+ * as indicated by the inline comments. Any difference in key presence,
order, or (for non-excluded keys) value
+ * will result in {@code false}.
+ *
+ * @param thatMetadata the metadata map to compare with
+ * @return {@code true} if the metadata maps are considered equal
according to the above rules; {@code false} otherwise
+ */
+ private boolean compareMetadata(SortedMap<String, String> thatMetadata) {
+ Iterator<Map.Entry<String, String>> thisIter =
this.metadata.entrySet().iterator();
+ Iterator<Map.Entry<String, String>> thatIter =
thatMetadata.entrySet().iterator();
+
+ boolean hasNext = thisIter.hasNext();
+ if (hasNext != thatIter.hasNext()) {
+ // hashCode will be different if there are an extra or less key
between two metadata.
+ return false;
+ }
+
+ while (hasNext) {
+ Map.Entry<String, String> thisEntry = thisIter.next();
+ Map.Entry<String, String> thatEntry = thatIter.next();
+
+ String key = thisEntry.getKey();
+ if (!key.equals(thatEntry.getKey())) {
+ // hashCode will be different if there are an extra or less
key between two metadata.
+ return false;
}
- if (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
+
+ if (!key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) &&
!key.equals(TIMESTAMP_KEY)) {
+ if (!Objects.equals(thisEntry.getValue(),
thatEntry.getValue())) {
+ return false;
+ }
+ }
+
+ hasNext = thisIter.hasNext();
+ if (hasNext != thatIter.hasNext()) {
+ // hashCode will be different if there are an extra or less
key between two metadata.
+ return false;
}
}
- return equals;
+ return true;
}
@Override
public int hashCode() {
int result = Objects.hash(getServiceName(), getHost(), getPort());
- for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
+
+ // use SortedMap as metadata type to ensure the hashCode calculation
is consistent with equals method.
+ for (Map.Entry<String, String> entry : this.metadata.entrySet()) {
+ String key = entry.getKey();
+ if (key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) ||
key.equals(TIMESTAMP_KEY)) {
continue;
}
result = 31 * result
Review Comment:
The hashCode calculation is incomplete. The line appears to be cut off and
doesn't include the actual hash contribution from the metadata entry.
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +310,77 @@ public boolean equals(Object o) {
return false;
}
DefaultServiceInstance that = (DefaultServiceInstance) o;
- boolean equals = Objects.equals(getServiceName(),
that.getServiceName())
- && Objects.equals(getHost(), that.getHost())
- && Objects.equals(getPort(), that.getPort());
- for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
- continue;
+ if (!Objects.equals(serviceName, that.serviceName)
+ || !Objects.equals(host, that.host)
+ || !Objects.equals(port, that.port)) {
+ return false;
+ }
+
+ // compare metadata.
+ return compareMetadata(that.metadata);
+ }
+
+ /**
+ * Compares this instance's metadata with another {@code SortedMap<String,
String>} for equality.
+ * <p>
+ * The comparison is performed in key order. For each key:
+ * <ul>
+ * <li>If the key is EXPORTED_SERVICES_REVISION_PROPERTY_NAME or
TIMESTAMP_KEY,
+ * only the presence and order of the key are checked; their values
are ignored.</li>
+ * <li>For all other keys, both the key and its value must match.</li>
+ * </ul>
+ * <p>
+ * This method is designed to ensure consistency with the {@code hashCode}
implementation,
+ * as indicated by the inline comments. Any difference in key presence,
order, or (for non-excluded keys) value
+ * will result in {@code false}.
+ *
+ * @param thatMetadata the metadata map to compare with
+ * @return {@code true} if the metadata maps are considered equal
according to the above rules; {@code false} otherwise
+ */
+ private boolean compareMetadata(SortedMap<String, String> thatMetadata) {
+ Iterator<Map.Entry<String, String>> thisIter =
this.metadata.entrySet().iterator();
+ Iterator<Map.Entry<String, String>> thatIter =
thatMetadata.entrySet().iterator();
+
+ boolean hasNext = thisIter.hasNext();
+ if (hasNext != thatIter.hasNext()) {
+ // hashCode will be different if there are an extra or less key
between two metadata.
+ return false;
+ }
+
+ while (hasNext) {
+ Map.Entry<String, String> thisEntry = thisIter.next();
+ Map.Entry<String, String> thatEntry = thatIter.next();
+
+ String key = thisEntry.getKey();
+ if (!key.equals(thatEntry.getKey())) {
+ // hashCode will be different if there are an extra or less
key between two metadata.
+ return false;
}
- if (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
+
+ if (!key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) &&
!key.equals(TIMESTAMP_KEY)) {
+ if (!Objects.equals(thisEntry.getValue(),
thatEntry.getValue())) {
+ return false;
+ }
+ }
+
+ hasNext = thisIter.hasNext();
+ if (hasNext != thatIter.hasNext()) {
+ // hashCode will be different if there are an extra or less
key between two metadata.
+ return false;
}
}
- return equals;
+ return true;
}
Review Comment:
[nitpick] The compareMetadata method is overly complex and contains
duplicate logic for checking iterator states. Consider simplifying by using a
more straightforward approach that filters out excluded keys first, then
compares the remaining entries directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
