Skip to content
This repository was archived by the owner on May 28, 2018. It is now read-only.

Commit b0de20e

Browse files
mgajdosmgajdos
committed
Merge pull request #175 from kppk/master
JERSEY-2891: ExceptionMapper not chosen correctly in case of multiple ExtendedExceptionMappers with same generic type
2 parents f145e34 + e8ae557 commit b0de20e

File tree

2 files changed

+242
-24
lines changed

2 files changed

+242
-24
lines changed

‎core-common/src/main/java/org/glassfish/jersey/internal/ExceptionMapperFactory.java‎

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,8 @@
4747
import java.util.Collection;
4848
import java.util.Comparator;
4949
import java.util.LinkedHashSet;
50-
import java.util.Map;
5150
import java.util.Set;
5251
import java.util.SortedSet;
53-
import java.util.TreeMap;
5452
import java.util.TreeSet;
5553
import java.util.logging.Level;
5654
import java.util.logging.Logger;
@@ -122,35 +120,29 @@ public <T extends Throwable> ExceptionMapper<T> find(final Class<T> type) {
122120

123121
@SuppressWarnings("unchecked")
124122
private <T extends Throwable> ExceptionMapper<T> find(final Class<T> type, final T exceptionInstance) {
125-
126-
final Map<Integer, ExceptionMapper<T>> orderedMappers = new TreeMap<Integer, ExceptionMapper<T>>();
127-
123+
ExceptionMapper<T> mapper = null;
124+
int minDistance = Integer.MAX_VALUE;
128125
for (final ExceptionMapperType mapperType : exceptionMapperTypes) {
129126
final int d = distance(type, mapperType.exceptionType);
130-
if (d >= 0) {
131-
orderedMappers.put(d, mapperType.mapper.getService());
132-
}
133-
}
134-
135-
if (orderedMappers.size() == 0) {
136-
return null;
137-
}
138-
139-
if (exceptionInstance != null) {
140-
for (final ExceptionMapper<T> mapper : orderedMappers.values()) {
141-
if (mapper instanceof ExtendedExceptionMapper) {
142-
final boolean mappable = ((ExtendedExceptionMapper<T>) mapper).isMappable(exceptionInstance);
143-
if (mappable) {
127+
if (d >= 0 && d <= minDistance) {
128+
final ExceptionMapper<T> candidateMapper = mapperType.mapper.getService();
129+
if (isMappable(exceptionInstance, candidateMapper)) {
130+
mapper = candidateMapper;
131+
minDistance = d;
132+
if (d == 0) {
133+
// slight optimization: if the distance is 0, it is already the best case, so we can exit
144134
return mapper;
145135
}
146-
} else {
147-
return mapper;
148136
}
149137
}
150-
return null;
151-
} else {
152-
return orderedMappers.values().iterator().next();
153138
}
139+
return mapper;
140+
}
141+
142+
private <T extends Throwable> boolean isMappable(T exceptionInstance, ExceptionMapper<T> mapper) {
143+
return exceptionInstance == null
144+
|| !(mapper instanceof ExtendedExceptionMapper)
145+
|| ((ExtendedExceptionMapper<T>) mapper).isMappable(exceptionInstance);
154146
}
155147

156148
/**

‎core-common/src/test/java/org/glassfish/jersey/internal/ExceptionMapperFactoryTest.java‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
/*
2+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
3+
*
4+
* Copyright (c) 2015 Oracle and/or its affiliates. All rights reserved.
5+
*
6+
* The contents of this file are subject to the terms of either the GNU
7+
* General Public License Version 2 only ("GPL") or the Common Development
8+
* and Distribution License("CDDL") (collectively, the "License"). You
9+
* may not use this file except in compliance with the License. You can
10+
* obtain a copy of the License at
11+
* http://glassfish.java.net/public/CDDL+GPL_1_1.html
12+
* or packager/legal/LICENSE.txt. See the License for the specific
13+
* language governing permissions and limitations under the License.
14+
*
15+
* When distributing the software, include this License Header Notice in each
16+
* file and include the License file at packager/legal/LICENSE.txt.
17+
*
18+
* GPL Classpath Exception:
19+
* Oracle designates this particular file as subject to the "Classpath"
20+
* exception as provided by Oracle in the GPL Version 2 section of the License
21+
* file that accompanied this code.
22+
*
23+
* Modifications:
24+
* If applicable, add the following below the License Header, with the fields
25+
* enclosed by brackets [] replaced by your own identifying information:
26+
* "Portions Copyright [year] [name of copyright owner]"
27+
*
28+
* Contributor(s):
29+
* If you wish your version of this file to be governed by only the CDDL or
30+
* only the GPL Version 2, indicate your decision by adding "[Contributor]
31+
* elects to include this software in this distribution under the [CDDL or GPL
32+
* Version 2] license." If you don't indicate a single choice of license, a
33+
* recipient has the option to distribute your version of this file under
34+
* either the CDDL, the GPL Version 2 or to extend the choice of license to
35+
* its licensees as provided above. However, if you add GPL Version 2 code
36+
* and therefore, elected the GPL Version 2 license, then the option applies
37+
* only if the new code is made subject to such option by the copyright
38+
* holder.
39+
*/
40+
41+
package org.glassfish.jersey.internal;
42+
43+
import org.glassfish.hk2.api.ServiceLocator;
44+
import org.glassfish.hk2.utilities.binding.AbstractBinder;
45+
import org.glassfish.jersey.internal.inject.Injections;
46+
import org.glassfish.jersey.spi.ExtendedExceptionMapper;
47+
import org.junit.Assert;
48+
import org.junit.Test;
49+
50+
import javax.inject.Singleton;
51+
import javax.ws.rs.core.Response;
52+
import javax.ws.rs.ext.ExceptionMapper;
53+
54+
/**
55+
* Unit test of {@link ExceptionMapperFactory}.
56+
*/
57+
public class ExceptionMapperFactoryTest {
58+
59+
private static class ExtendedExceptionMappers extends AbstractBinder {
60+
61+
@Override
62+
protected void configure() {
63+
bind(IllegalArgumentExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
64+
bind(IllegalStateExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
65+
}
66+
67+
}
68+
69+
private static class AllMappers extends AbstractBinder {
70+
71+
@Override
72+
protected void configure() {
73+
bind(IllegalArgumentExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
74+
bind(IllegalStateExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
75+
bind(RuntimeExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
76+
}
77+
78+
}
79+
80+
81+
/**
82+
* Test spec:
83+
* <p/>
84+
* setup:<br/>
85+
* - have two extended exception mappers, order matters<br/>
86+
* - both using the same generic type (RuntimeException)<br/>
87+
* - first mapper return isMappable true only to IllegalArgumentException<br/>
88+
* - second mapper return isMappable true only to IllegalStateException<br/>
89+
* <br/>
90+
* when:<br/>
91+
* - {@link ExceptionMapperFactory#findMapping(Throwable)} with IllegalArgumentException instance<br/>
92+
* <br/>
93+
* then:<br/>
94+
* - exception mapper factory returns IllegalArgumentExceptionMapper<br/>
95+
* <p/>
96+
* why:<br/>
97+
* - IllegalArgumentException has the same distance (1) for both exception mappers generic type (RuntimeException),
98+
* but IllegalArgumentException's isMappable return true, so it is the winner
99+
*
100+
* @throws Exception unexpected - if anything goes wrong, the test fails
101+
*/
102+
@Test
103+
public void testFindMappingExtendedExceptions() throws Exception {
104+
ServiceLocator serviceLocator = Injections.createLocator(new ExtendedExceptionMappers());
105+
ExceptionMapperFactory mapperFactory = new ExceptionMapperFactory(serviceLocator);
106+
107+
ExceptionMapper mapper = mapperFactory.findMapping(new IllegalArgumentException());
108+
109+
Assert.assertTrue("IllegalArgumentExceptionMapper should be returned",
110+
mapper instanceof IllegalArgumentExceptionMapper);
111+
}
112+
113+
/**
114+
* Test spec:
115+
* <p/>
116+
* setup:<br/>
117+
* - have 3 exception mappers, order matters<br/>
118+
* - first is *not* extended mapper typed to RuntimeException
119+
* - second and third are extended mappers type to RuntimeException
120+
* <br/>
121+
* when:<br/>
122+
* - {@link ExceptionMapperFactory#findMapping(Throwable)} invoked with RuntimeException instance<br/>
123+
* then: <br/>
124+
* - exception mapper factory returns RuntimeExceptionMapper<br/>
125+
* <p/>
126+
* why:<br/>
127+
* - RuntimeException mapper has distance 0 for RuntimeException, it is not extended mapper, so it will be chosen
128+
* immediately, cause there is no better option possible
129+
*
130+
* @throws Exception unexpected - if anything goes wrong, the test fails
131+
*/
132+
@Test
133+
public void testFindMapping() throws Exception {
134+
ServiceLocator serviceLocator = Injections.createLocator(new AllMappers());
135+
ExceptionMapperFactory mapperFactory = new ExceptionMapperFactory(serviceLocator);
136+
137+
ExceptionMapper<RuntimeException> mapper = mapperFactory.findMapping(new RuntimeException());
138+
139+
Assert.assertTrue("RuntimeExceptionMapper should be returned", mapper instanceof RuntimeExceptionMapper);
140+
}
141+
142+
/**
143+
* Test spec: <br/>
144+
* <p/>
145+
* setup:<br/>
146+
* - have 2 extended mappers, order matters<br/>
147+
* - first mapper return isMappable true only to IllegalArgumentException<br/>
148+
* - second mapper return isMappable true only to IllegalStateException<br/>
149+
* <br/>
150+
* when:<br/>
151+
* - {@link ExceptionMapperFactory#find(Class)} invoked with IllegalArgumentException.class<br/>
152+
* then:<br/>
153+
* - exception mapper factory returns IllegalArgumentExceptionMapper<br/>
154+
* <p/>
155+
* why:<br/>
156+
* - both exception mappers have distance 1 to IllegalArgumentException, we don't have instance of the
157+
* IllegalArgumentException, so the isMappable check is not used and both are accepted, the later accepted is
158+
* the winner
159+
*
160+
* @throws Exception unexpected - if anything goes wrong, the test fails
161+
*/
162+
@Test
163+
public void testFindExtendedExceptions() throws Exception {
164+
ServiceLocator serviceLocator = Injections.createLocator(new ExtendedExceptionMappers());
165+
ExceptionMapperFactory mapperFactory = new ExceptionMapperFactory(serviceLocator);
166+
167+
ExceptionMapper mapper = mapperFactory.find(IllegalArgumentException.class);
168+
169+
Assert.assertTrue("IllegalStateExceptionMapper should be returned",
170+
mapper instanceof IllegalStateExceptionMapper);
171+
}
172+
173+
/**
174+
* Extended Exception Mapper which has RuntimeException as generic type and isMappable returns true if the
175+
* exception is instance of IllegalArgumentException.
176+
*/
177+
private static class IllegalArgumentExceptionMapper implements ExtendedExceptionMapper<RuntimeException> {
178+
179+
@Override
180+
public boolean isMappable(RuntimeException exception) {
181+
return exception instanceof IllegalArgumentException;
182+
}
183+
184+
@Override
185+
public Response toResponse(RuntimeException exception) {
186+
return Response
187+
.status(Response.Status.BAD_REQUEST)
188+
.build();
189+
}
190+
191+
}
192+
193+
/**
194+
* Extended Exception Mapper which has RuntimeException as generic type and isMappable returns true if the
195+
* exception is instance of IllegalStateException.
196+
*/
197+
private static class IllegalStateExceptionMapper implements ExtendedExceptionMapper<RuntimeException> {
198+
199+
@Override
200+
public boolean isMappable(RuntimeException exception) {
201+
return exception instanceof IllegalStateException;
202+
}
203+
204+
@Override
205+
public Response toResponse(RuntimeException exception) {
206+
return Response
207+
.status(Response.Status.SERVICE_UNAVAILABLE)
208+
.build();
209+
}
210+
211+
}
212+
213+
/**
214+
* Exception Mapper which has RuntimeException as generic type.
215+
*/
216+
private static class RuntimeExceptionMapper implements ExceptionMapper<RuntimeException> {
217+
218+
@Override
219+
public Response toResponse(RuntimeException exception) {
220+
return Response
221+
.status(Response.Status.INTERNAL_SERVER_ERROR)
222+
.build();
223+
}
224+
225+
}
226+
}

0 commit comments

Comments
 (0)