Mark, On 3/17/15 4:25 PM, Mark Thomas wrote: >>> On 3/9/15 12:19 PM, Christopher Schultz wrote: >>>> All, >>>> >>>> I was looking at https://bz.apache.org/bugzilla/show_bug.cgi?id=57676 (a >>>> simple proposed patch to improve an error message) and I was trying to >>>> figure out what to do with a bit of code in addEndpoint. Reading the >>>> code, I think I've spotted a problem: >>>> >>>> [187] UriTemplate uriTemplate = new UriTemplate(path); >>>> if (uriTemplate.hasParameters()) { >>>> Integer key = Integer.valueOf(uriTemplate.getSegmentCount()); >>>> SortedSet<TemplatePathMatch> templateMatches = >>>> configTemplateMatchMap.get(key); >>>> if (templateMatches == null) { >>>> // Ensure that if concurrent threads execute this block >>>> they >>>> // both end up using the same TreeSet instance >>>> templateMatches = new TreeSet<>( >>>> TemplatePathMatchComparator.getInstance()); >>>> configTemplateMatchMap.putIfAbsent(key, templateMatches); >>>> templateMatches = configTemplateMatchMap.get(key); >>>> } >>>> [200] if (!templateMatches.add(new TemplatePathMatch(sec, >>>> uriTemplate))) { >>>> // Duplicate uriTemplate; >>>> throw new DeploymentException( >>>> sm.getString("serverContainer.duplicatePaths", >>>> path, >>>> sec.getEndpointClass(), >>>> sec.getEndpointClass())); >>>> } >>>> } >>>> >>>> The problem is on line 200, where we check to see if >>>> templateMatches.add() returns false (meaning that the object we added to >>>> the set replaced one that was already there). >>>> >>>> I don't believe this branch can /ever/ be reached because the >>>> TemplatePathMatch class doesn't override Object.equals(), thus no two >>>> objects of that type will ever equal each other. Therefore, a new >>>> TemplatePathMatch object will always be added to the set. > > Your analysis is flawed. > > The only place values are added to configTemplateMatchMap is on line > 197. The value object is always the TreeSet constructed on the previous > line with a TemplatePathMatchComparator. > > That TemplatePathMatch doesn't override equals is therefore irrelevant. > The Comparator will always be used.
+1 I wasn't considering the comparator being used by the collection itself. Thanks, -chris
signature.asc
Description: OpenPGP digital signature
