Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2280256450
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +311,34 @@ 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 (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
- }
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
}
- return equals;
+ // create filtered metadata of 'this' instance.
+ SortedMap<String, String> filteredMetadataOfThis =
this.getSortedMetadata();
+
filteredMetadataOfThis.remove(EXPORTED_SERVICES_REVISION_PROPERTY_NAME);
+ filteredMetadataOfThis.remove(TIMESTAMP_KEY);
+
+ // create filtered metadata of 'that' instance.
+ SortedMap<String, String> filteredMetadataOfThat =
that.getSortedMetadata();
Review Comment:
The equals method modifies the metadata maps by calling remove() on them,
but getSortedMetadata() returns a copy that should not be modified. This could
cause side effects if the copy is unexpectedly shared or if the implementation
changes. Consider creating explicit copies for filtering instead of relying on
getSortedMetadata().
```suggestion
SortedMap<String, String> filteredMetadataOfThis = new
TreeMap<>(this.getSortedMetadata());
filteredMetadataOfThis.remove(EXPORTED_SERVICES_REVISION_PROPERTY_NAME);
filteredMetadataOfThis.remove(TIMESTAMP_KEY);
// create filtered metadata of 'that' instance.
SortedMap<String, String> filteredMetadataOfThat = new
TreeMap<>(that.getSortedMetadata());
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +311,34 @@ 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 (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
- }
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
}
- return equals;
+ // create filtered metadata of 'this' instance.
+ SortedMap<String, String> filteredMetadataOfThis =
this.getSortedMetadata();
+
filteredMetadataOfThis.remove(EXPORTED_SERVICES_REVISION_PROPERTY_NAME);
+ filteredMetadataOfThis.remove(TIMESTAMP_KEY);
+
+ // create filtered metadata of 'that' instance.
+ SortedMap<String, String> filteredMetadataOfThat =
that.getSortedMetadata();
+
filteredMetadataOfThat.remove(EXPORTED_SERVICES_REVISION_PROPERTY_NAME);
+ filteredMetadataOfThat.remove(TIMESTAMP_KEY);
+
+ // compare filtered metadata.
+ return filteredMetadataOfThis.equals(filteredMetadataOfThat);
}
@Override
public int hashCode() {
int result = Objects.hash(getServiceName(), getHost(), getPort());
+
+ // The hashCode result is calculated by the same order, as the
metadata is a SortedMap.
for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
+ String key = entry.getKey();
+ if (key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) ||
key.equals(TIMESTAMP_KEY)) {
Review Comment:
The filtering logic is duplicated between equals() and hashCode() methods.
Consider extracting this into a helper method that returns filtered metadata to
reduce code duplication and ensure consistency.
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +311,34 @@ 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 (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
- }
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
}
- return equals;
+ // create filtered metadata of 'this' instance.
+ SortedMap<String, String> filteredMetadataOfThis =
this.getSortedMetadata();
+
filteredMetadataOfThis.remove(EXPORTED_SERVICES_REVISION_PROPERTY_NAME);
+ filteredMetadataOfThis.remove(TIMESTAMP_KEY);
+
+ // create filtered metadata of 'that' instance.
+ SortedMap<String, String> filteredMetadataOfThat =
that.getSortedMetadata();
+
filteredMetadataOfThat.remove(EXPORTED_SERVICES_REVISION_PROPERTY_NAME);
+ filteredMetadataOfThat.remove(TIMESTAMP_KEY);
Review Comment:
Similar to the previous issue, modifying the result of getSortedMetadata()
with remove() operations is risky. The method should create explicit filtered
copies rather than modifying returned maps.
```suggestion
SortedMap<String, String> filteredMetadataOfThis = new TreeMap<>();
for (Map.Entry<String, String> entry :
this.getSortedMetadata().entrySet()) {
String key = entry.getKey();
if (!key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) &&
!key.equals(TIMESTAMP_KEY)) {
filteredMetadataOfThis.put(key, entry.getValue());
}
}
// create filtered metadata of 'that' instance.
SortedMap<String, String> filteredMetadataOfThat = new TreeMap<>();
for (Map.Entry<String, String> entry :
that.getSortedMetadata().entrySet()) {
String key = entry.getKey();
if (!key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) &&
!key.equals(TIMESTAMP_KEY)) {
filteredMetadataOfThat.put(key, entry.getValue());
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
