Skip to content

Commit e529ff5

Browse files
graemerochergraemerocher
committed
Make sure two root mappings are equal in compareTo. Fixes #10128 
1 parent b3a97bb commit e529ff5

File tree

2 files changed

+136
-15
lines changed

2 files changed

+136
-15
lines changed

‎grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java‎

Lines changed: 123 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@
4949
import org.grails.web.servlet.mvc.GrailsWebRequest;
5050
import org.grails.web.servlet.mvc.exceptions.ControllerExecutionException;
5151
import org.grails.web.util.WebUtils;
52+
import org.slf4j.Logger;
53+
import org.slf4j.LoggerFactory;
5254
import org.springframework.http.HttpMethod;
5355
import org.springframework.util.Assert;
5456
import org.springframework.validation.Errors;
@@ -79,7 +81,7 @@ public class RegexUrlMapping extends AbstractUrlMapping {
7981
private Map<Integer, List<Pattern>> patternByTokenCount = new HashMap<Integer, List<Pattern>>();
8082
private UrlMappingData urlData;
8183
private static final String DEFAULT_ENCODING = "UTF-8";
82-
private static final Log LOG = LogFactory.getLog(RegexUrlMapping.class);
84+
private static final Logger LOG = LoggerFactory.getLogger(RegexUrlMapping.class);
8385
public static final Pattern DOUBLE_WILDCARD_PATTERN = Pattern.compile("\\(\\*\\*?\\)\\??");
8486
public static final Pattern OPTIONAL_EXTENSION_WILDCARD_PATTERN = Pattern.compile("[^/]+\\(\\.\\(\\*\\)\\)");
8587

@@ -773,46 +775,86 @@ public int compareTo(Object o) {
773775

774776
UrlMapping other = (UrlMapping) o;
775777

778+
// this wild card count
776779
final int thisStaticTokenCount = getStaticTokenCount(this);
780+
final int thisSingleWildcardCount = getSingleWildcardCount(this);
781+
final int thisDoubleWildcardCount = getDoubleWildcardCount(this);
782+
783+
// the other wild card count
777784
final int otherStaticTokenCount = getStaticTokenCount(other);
778785
final int otherSingleWildcardCount = getSingleWildcardCount(other);
779-
final int thisSingleWildcardCount = getSingleWildcardCount(this);
780786
final int otherDoubleWildcardCount = getDoubleWildcardCount(other);
781-
final int thisDoubleWildcardCount = getDoubleWildcardCount(this);
787+
782788
final boolean hasWildCards = thisDoubleWildcardCount > 0 || thisSingleWildcardCount > 0;
783789
final boolean otherHasWildCards = otherDoubleWildcardCount > 0 || otherSingleWildcardCount > 0;
784790

785791
// Always prioritise the / root mapping
786-
if(thisStaticTokenCount == 0 && thisSingleWildcardCount == 0 && thisDoubleWildcardCount == 0) {
792+
boolean isThisRoot = thisStaticTokenCount == 0 && thisSingleWildcardCount == 0 && thisDoubleWildcardCount == 0;
793+
boolean isThatRoot = otherStaticTokenCount == 0 && otherDoubleWildcardCount == 0 && otherSingleWildcardCount == 0;
794+
795+
if(isThisRoot && isThatRoot) {
796+
if(LOG.isDebugEnabled()) {
797+
LOG.debug("Mapping [{}] has equal precedence with mapping [{}]", this.toString(), other.toString());
798+
}
799+
return 0;
800+
}
801+
else if(isThisRoot) {
802+
if(LOG.isDebugEnabled()) {
803+
LOG.debug("Mapping [{}] has a higher precedence than [{}] because it is the root", this.toString(), other.toString());
804+
}
787805
return 1;
788806
}
789-
if(otherStaticTokenCount == 0 && otherSingleWildcardCount == 0 && otherDoubleWildcardCount == 0) {
807+
else if(isThatRoot) {
808+
if(LOG.isDebugEnabled()) {
809+
LOG.debug("Mapping [{}] has a lower precedence than [{}] because the latter is the root", this.toString(), other.toString());
810+
}
790811
return -1;
791812
}
792813

793-
if (otherStaticTokenCount ==0 && thisStaticTokenCount > 0) {
814+
if (otherStaticTokenCount == 0 && thisStaticTokenCount > 0) {
815+
if(LOG.isDebugEnabled()) {
816+
LOG.debug("Mapping [{}] has a higher precedence than [{}] because it has more path tokens", this.toString(), other.toString());
817+
}
794818
return 1;
795819
}
796-
if (thisStaticTokenCount ==0 && otherStaticTokenCount>0) {
820+
821+
if (thisStaticTokenCount == 0 && otherStaticTokenCount > 0) {
822+
if(LOG.isDebugEnabled()) {
823+
LOG.debug("Mapping [{}] has a lower precedence than [{}] because it has fewer path tokens", this.toString(), other.toString());
824+
}
797825
return -1;
798826
}
799827

800828
final int thisStaticAndWildcardTokenCount = getStaticAndWildcardTokenCount(this);
801829
final int otherStaticAndWildcardTokenCount = getStaticAndWildcardTokenCount(other);
830+
802831
if (otherStaticAndWildcardTokenCount==0 && thisStaticAndWildcardTokenCount>0) {
832+
if(LOG.isDebugEnabled()) {
833+
LOG.debug("Mapping [{}] has a higher precedence than [{}] because it has more path tokens [{} vs {}]", this.toString(), other.toString(), thisStaticAndWildcardTokenCount, otherStaticAndWildcardTokenCount);
834+
}
803835
return 1;
804836
}
805837
if (thisStaticAndWildcardTokenCount==0 && otherStaticAndWildcardTokenCount>0) {
838+
if(LOG.isDebugEnabled()) {
839+
LOG.debug("Mapping [{}] has a higher precedence than [{}] because the latter has more path tokens [{} vs {}]", this.toString(), other.toString(), thisStaticAndWildcardTokenCount, otherStaticAndWildcardTokenCount);
840+
}
806841
return -1;
807842
}
808843

809844
final int staticDiff = thisStaticTokenCount - otherStaticTokenCount;
810845
if (staticDiff < 0 && !otherHasWildCards) {
811-
return staticDiff;
846+
if(LOG.isDebugEnabled()) {
847+
LOG.debug("Mapping [{}] has a lower precedence than [{}] because the latter has more concrete path tokens [{} vs {}]", this.toString(), other.toString(), thisStaticTokenCount, otherStaticTokenCount);
848+
}
849+
return -1;
812850
}
813851
else if(staticDiff > 0 && !hasWildCards) {
814-
return staticDiff;
852+
if(LOG.isDebugEnabled()) {
853+
LOG.debug("Mapping [{}] has a higher precedence than [{}] because it has more concrete path tokens [{} vs {}]", this.toString(), other.toString(), thisStaticTokenCount, otherStaticTokenCount);
854+
}
855+
return 1;
815856
}
857+
816858
String[] thisTokens = getUrlData().getTokens();
817859
String[] otherTokens = other.getUrlData().getTokens();
818860
final int thisTokensLength = thisTokens.length;
@@ -826,40 +868,107 @@ else if(staticDiff > 0 && !hasWildCards) {
826868
boolean thisTokenIsWildcard = !thisHasMoreTokens || isSingleWildcard(thisTokens[i]);
827869
boolean otherTokenIsWildcard = !otherHasMoreTokens || isSingleWildcard(otherTokens[i]);
828870
if (thisTokenIsWildcard && !otherTokenIsWildcard) {
871+
if(LOG.isDebugEnabled()) {
872+
LOG.debug("Mapping [{}] has a lower precedence than [{}] because the latter contains more concrete tokens", this.toString(), other.toString());
873+
}
829874
return -1;
830875
}
831876
if (!thisTokenIsWildcard && otherTokenIsWildcard) {
877+
if(LOG.isDebugEnabled()) {
878+
LOG.debug("Mapping [{}] has a higher precedence than [{}] because it contains more concrete tokens", this.toString(), other.toString());
879+
}
832880
return 1;
833881
}
834882
}
835883

836884
final int doubleWildcardDiff = otherDoubleWildcardCount - thisDoubleWildcardCount;
837-
if (doubleWildcardDiff != 0) return doubleWildcardDiff;
885+
if (doubleWildcardDiff != 0) {
886+
if(LOG.isDebugEnabled()) {
887+
if(doubleWildcardDiff > 0) {
888+
LOG.debug("Mapping [{}] has a higher precedence than [{}] due containing more double wild cards [{} vs. {}]", this.toString(), other.toString(), thisDoubleWildcardCount, otherDoubleWildcardCount);
889+
}
890+
else if(doubleWildcardDiff < 0) {
891+
LOG.debug("Mapping [{}] has a lower precedence than [{}] due to the latter containing more double wild cards [{} vs. {}]", this.toString(), other.toString(), thisDoubleWildcardCount, otherDoubleWildcardCount);
892+
}
893+
}
894+
return doubleWildcardDiff;
895+
}
838896

839897
final int singleWildcardDiff = otherSingleWildcardCount - thisSingleWildcardCount;
840-
if (singleWildcardDiff != 0) return singleWildcardDiff;
898+
if (singleWildcardDiff != 0) {
899+
if(LOG.isDebugEnabled()) {
900+
if(singleWildcardDiff > 0) {
901+
LOG.debug("Mapping [{}] has a higher precedence than [{}] because it contains more single wild card matches [{} vs. {}]", this.toString(), other.toString(), thisSingleWildcardCount, otherSingleWildcardCount);
902+
}
903+
else if(singleWildcardDiff < 0) {
904+
LOG.debug("Mapping [{}] has a lower precedence than [{}] due to the latter containing more single wild card matches[{} vs. {}]", this.toString(), other.toString(), thisSingleWildcardCount, otherSingleWildcardCount);
905+
}
906+
}
907+
return singleWildcardDiff;
908+
}
841909

842-
int constraintDiff = getAppliedConstraintsCount(this) - getAppliedConstraintsCount(other);
843-
if (constraintDiff != 0) return constraintDiff;
910+
int thisConstraintCount = getAppliedConstraintsCount(this);
911+
int thatConstraintCount = getAppliedConstraintsCount(other);
912+
int constraintDiff = thisConstraintCount - thatConstraintCount;
913+
if (constraintDiff != 0) {
914+
if(LOG.isDebugEnabled()) {
915+
if(constraintDiff > 0) {
916+
LOG.debug("Mapping [{}] has a higher precedence than [{}] since it defines more constraints [{} vs. {}]", this.toString(), other.toString(), thisConstraintCount, thatConstraintCount);
917+
}
918+
else if(constraintDiff < 0) {
919+
LOG.debug("Mapping [{}] has a lower precedence than [{}] since the latter defines more constraints [{} vs. {}]", this.toString(), other.toString(), thisConstraintCount, thatConstraintCount);
920+
}
921+
}
922+
return constraintDiff;
923+
}
844924

845925
int allDiff = (thisStaticTokenCount - otherStaticTokenCount) + (thisSingleWildcardCount - otherSingleWildcardCount) + (thisDoubleWildcardCount - otherDoubleWildcardCount);
846926
if(allDiff != 0) {
927+
if(LOG.isDebugEnabled()) {
928+
if(allDiff > 0) {
929+
LOG.debug("Mapping [{}] has a higher precedence than [{}] due to the overall diff", this.toString(), other.toString());
930+
}
931+
else if(allDiff < 0) {
932+
LOG.debug("Mapping [{}] has a lower precedence than [{}] due to the overall diff", this.toString(), other.toString());
933+
}
934+
}
847935
return allDiff;
848936
}
849937

850938
String thisVersion = getVersion();
851939
String thatVersion = other.getVersion();
852940
if((thisVersion.equals(thatVersion))) {
941+
if(LOG.isDebugEnabled()) {
942+
LOG.debug("Mapping [{}] has equal precedence with mapping [{}]", this.toString(), other.toString());
943+
}
853944
return 0;
854945
}
855946
else if(thisVersion.equals(ANY_VERSION) && !thatVersion.equals(ANY_VERSION)) {
947+
if(LOG.isDebugEnabled()) {
948+
LOG.debug("Mapping [{}] has a lower precedence than [{}] due to version precedence [{} vs {}]", this.toString(), other.toString(), thisVersion, thatVersion);
949+
}
856950
return -1;
857951
}
858952
else if(!thisVersion.equals(ANY_VERSION) && thatVersion.equals(ANY_VERSION)) {
953+
if(LOG.isDebugEnabled()) {
954+
LOG.debug("Mapping [{}] has a higher precedence than [{}] due to version precedence [{} vs {}]", this.toString(), other.toString(), thisVersion, thatVersion);
955+
}
859956
return 1;
860957
}
861958
else {
862-
return new VersionComparator().compare(thisVersion, thatVersion);
959+
int i = new VersionComparator().compare(thisVersion, thatVersion);
960+
if(LOG.isDebugEnabled()) {
961+
if(i > 0) {
962+
LOG.debug("Mapping [{}] has a higher precedence than [{}] due to version precedence [{} vs. {}]", this.toString(), other.toString(), thisVersion, thatVersion);
963+
}
964+
else if(i < 0) {
965+
LOG.debug("Mapping [{}] has a lower precedence than [{}] due to version precedence [{} vs. {}]", this.toString(), other.toString(), thisVersion, thatVersion);
966+
}
967+
else {
968+
LOG.debug("Mapping [{}] has equal precedence with mapping [{}]", this.toString(), other.toString());
969+
}
970+
}
971+
return i;
863972
}
864973
}
865974

‎grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/RegexUrlMappingTests.groovy‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,19 @@ class RegexUrlMappingTests {
4141
def m8 = new RegexUrlMapping(parser.parse("/foo/(*)/(*)"), "test", null, null, null, null, null, UrlMapping.ANY_VERSION,null, servletContext)
4242
def m9 = new RegexUrlMapping(parser.parse("/(*)/(*)/bar"), "test", null, null, null, null, null, UrlMapping.ANY_VERSION,null, servletContext)
4343
def m10 = new RegexUrlMapping(parser.parse("/(*)/(*)/(*)"), "test", null, null, null, null, null, UrlMapping.ANY_VERSION,null, servletContext)
44-
44+
def m11 = new RegexUrlMapping(parser.parse("/"), "test", null, null, null, null, null, UrlMapping.ANY_VERSION,null, servletContext)
45+
46+
assertTrue m1.compareTo(m1) == 0
47+
assertTrue m2.compareTo(m2) == 0
48+
assertTrue m3.compareTo(m3) == 0
49+
assertTrue m4.compareTo(m4) == 0
50+
assertTrue m5.compareTo(m5) == 0
51+
assertTrue m6.compareTo(m6) == 0
52+
assertTrue m7.compareTo(m7) == 0
53+
assertTrue m8.compareTo(m8) == 0
54+
assertTrue m9.compareTo(m9) == 0
55+
assertTrue m2.compareTo(m11) == 0
56+
assertTrue m11.compareTo(m2) == 0
4557
assertTrue m1.compareTo(m2) < 0
4658
assertTrue m1.compareTo(m3) < 0
4759
assertTrue m1.compareTo(m4) < 0

0 commit comments

Comments
 (0)