Skip to content

Commit

Permalink
dcache-webadmin: fix comparison semantics
Browse files Browse the repository at this point in the history
Java 1.7 now enforces comparison contracts on sort, and will throw an IllegalArgument exception if the implementation of compareTo violates it.

Several comparisons used in the webadmin beans did not adhere to this contract.  For instance, something like:

a.name.compareTo(b.name) + a.domain.compareTo(b.domain) is incorrect, since this could possibly result in 0 if the two parts evaluate to oppositely signed 1s, when in fact 0 should be reserved for equality.

This patch reimplements all compareTo in the bean classes, modernizing them with a call to Guava's ComparisonChain.

Testing:

Reran junit tests.  Deployed and rechecked.

Target: master
Request: 2.6
Patch: http://rb.dcache.org/r/6307/
Bug: http://rt.dcache.org/Ticket/Display.html?id=8145
Require-notes: yes
Require-book: no
Acked-by: Tigran

RELEASE NOTES:

Fixes bug which throws java.lang.IllegalArgumentException: Comparison method violates its general contract! when data is being sorted in certain webadmin pages.
  • Loading branch information
alrossi committed Dec 5, 2013
1 parent 110a50c commit 5c788be
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 9 deletions.
@@ -1,5 +1,8 @@
package org.dcache.webadmin.view.beans;

import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;

import java.io.Serializable;

/**
Expand Down Expand Up @@ -94,7 +97,11 @@ public boolean equals(Object other) {

@Override
public int compareTo(CellServicesBean other) {
return (getName().compareTo(other.getName()) +
getDomainName().compareTo(other.getDomainName()));
return ComparisonChain.start()
.compare(getName(), other.getName(),
Ordering.natural().nullsLast())
.compare(getDomainName(), other.getDomainName(),
Ordering.natural().nullsLast())
.result();
}
}
@@ -1,5 +1,8 @@
package org.dcache.webadmin.view.beans;

import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;

import java.io.Serializable;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -57,6 +60,9 @@ public boolean equals(Object other) {

@Override
public int compareTo(PoolAdminBean other) {
return getGroupName().compareTo(other.getGroupName());
return ComparisonChain.start()
.compare(getGroupName(), other.getGroupName(),
Ordering.natural().nullsLast())
.result();
}
}
@@ -1,5 +1,8 @@
package org.dcache.webadmin.view.beans;

import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;

import java.io.Serializable;

/**
Expand Down Expand Up @@ -58,7 +61,11 @@ public boolean equals(Object other) {

@Override
public int compareTo(PoolCommandBean other) {
return (getName().compareTo(other.getName()) +
getDomain().compareTo(other.getDomain()));
return ComparisonChain.start()
.compare(getName(), other.getName(),
Ordering.natural().nullsLast())
.compare(getDomain(), other.getDomain(),
Ordering.natural().nullsLast())
.result();
}
}
@@ -1,5 +1,8 @@
package org.dcache.webadmin.view.beans;

import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;

import java.io.Serializable;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -170,6 +173,9 @@ public boolean equals(Object other) {

@Override
public int compareTo(PoolGroupBean other) {
return getName().compareTo(other.getName());
return ComparisonChain.start()
.compare(getName(), other.getName(),
Ordering.natural().nullsLast())
.result();
}
}
@@ -1,5 +1,8 @@
package org.dcache.webadmin.view.beans;

import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;

import java.io.Serializable;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -68,7 +71,11 @@ public boolean equals(Object other) {

@Override
public int compareTo(PoolQueueBean other) {
return (getName().compareTo(other.getName()) +
getDomainName().compareTo(other.getDomainName()));
return ComparisonChain.start()
.compare(getName(), other.getName(),
Ordering.natural().nullsLast())
.compare(getDomainName(), other.getDomainName(),
Ordering.natural().nullsLast())
.result();
}
}
@@ -1,5 +1,8 @@
package org.dcache.webadmin.view.beans;

import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -195,7 +198,10 @@ public void setPoolMode(PoolV2Mode poolMode) {

@Override
public int compareTo(PoolSpaceBean other) {
return this.getName().compareTo(other.getName());
return ComparisonChain.start()
.compare(getName(), other.getName(),
Ordering.natural().nullsLast())
.result();
}

@Override
Expand Down

0 comments on commit 5c788be

Please sign in to comment.