Skip to content

Commit dd5ce54

Browse files
authored
Fix(core): Prevent redundant schema resolution by fixing AnnotatedType equality (#4975)
* Fix: Prevent redundant schema resolution by fixing AnnotatedType equality #4965 addresses mutability of ctxAnnotations, JDK/internal annotation noice and order-sensible annotations.
1 parent 4954f52 commit dd5ce54

File tree

3 files changed

+204
-39
lines changed

3 files changed

+204
-39
lines changed

modules/swagger-core/src/main/java/io/swagger/v3/core/converter/AnnotatedType.java

Lines changed: 27 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@
88
import java.lang.reflect.Type;
99
import java.util.ArrayList;
1010
import java.util.Arrays;
11+
import java.util.Comparator;
1112
import java.util.List;
1213
import java.util.Objects;
1314
import java.util.function.Function;
15+
import java.util.stream.Collectors;
1416

1517
public class AnnotatedType {
1618
private Type type;
@@ -155,11 +157,11 @@ public AnnotatedType name(String name) {
155157
}
156158

157159
public Annotation[] getCtxAnnotations() {
158-
return ctxAnnotations;
160+
return ctxAnnotations == null ? null : Arrays.copyOf(ctxAnnotations, ctxAnnotations.length);
159161
}
160162

161163
public void setCtxAnnotations(Annotation[] ctxAnnotations) {
162-
this.ctxAnnotations = ctxAnnotations;
164+
this.ctxAnnotations = ctxAnnotations == null ? null : Arrays.copyOf(ctxAnnotations, ctxAnnotations.length);
163165
}
164166

165167
public AnnotatedType ctxAnnotations(Annotation[] ctxAnnotations) {
@@ -241,49 +243,35 @@ public AnnotatedType propertyName(String propertyName) {
241243
return this;
242244
}
243245

246+
private List<Annotation> getProcessedAnnotations(Annotation[] annotations) {
247+
if (annotations == null || annotations.length == 0) {
248+
return new ArrayList<>();
249+
}
250+
return Arrays.stream(annotations)
251+
.filter(a -> {
252+
String pkg = a.annotationType().getPackage().getName();
253+
return !pkg.startsWith("java.") && !pkg.startsWith("jdk.") && !pkg.startsWith("sun.");
254+
})
255+
.sorted(Comparator.comparing(a -> a.annotationType().getName()))
256+
.collect(Collectors.toList());
257+
}
258+
244259
@Override
245260
public boolean equals(Object o) {
246-
if (this == o) {
247-
return true;
248-
}
249-
if (!(o instanceof AnnotatedType)) {
250-
return false;
251-
}
261+
if (this == o) return true;
262+
if (!(o instanceof AnnotatedType)) return false;
252263
AnnotatedType that = (AnnotatedType) o;
253-
254-
if ((type == null && that.type != null) || (type != null && that.type == null)) {
255-
return false;
256-
}
257-
258-
if (type != null && that.type != null && !type.equals(that.type)) {
259-
return false;
260-
}
261-
return Arrays.equals(this.ctxAnnotations, that.ctxAnnotations);
264+
List<Annotation> thisAnnotatinons = getProcessedAnnotations(this.ctxAnnotations);
265+
List<Annotation> thatAnnotatinons = getProcessedAnnotations(that.ctxAnnotations);
266+
return includePropertiesWithoutJSONView == that.includePropertiesWithoutJSONView &&
267+
Objects.equals(type, that.type) &&
268+
Objects.equals(thisAnnotatinons, thatAnnotatinons) &&
269+
Objects.equals(jsonViewAnnotation, that.jsonViewAnnotation);
262270
}
263271

264-
265272
@Override
266273
public int hashCode() {
267-
if (ctxAnnotations == null || ctxAnnotations.length == 0) {
268-
return Objects.hash(type, "fixed");
269-
}
270-
List<Annotation> meaningfulAnnotations = new ArrayList<>();
271-
272-
boolean hasDifference = false;
273-
for (Annotation a: ctxAnnotations) {
274-
if(!a.annotationType().getName().startsWith("sun") && !a.annotationType().getName().startsWith("jdk")) {
275-
meaningfulAnnotations.add(a);
276-
} else {
277-
hasDifference = true;
278-
}
279-
}
280-
int result = 1;
281-
result = 31 * result + (type == null ? 0 : Objects.hash(type, "fixed"));
282-
if (hasDifference) {
283-
result = 31 * result + meaningfulAnnotations.hashCode();
284-
} else {
285-
result = 31 * result + Arrays.hashCode(ctxAnnotations);
286-
}
287-
return result;
274+
List<Annotation> processedAnnotations = getProcessedAnnotations(this.ctxAnnotations);
275+
return Objects.hash(type, jsonViewAnnotation, includePropertiesWithoutJSONView, processedAnnotations);
288276
}
289277
}
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
package io.swagger.v3.core.converting;
2+
3+
import io.swagger.v3.core.converter.AnnotatedType;
4+
import io.swagger.v3.core.converter.ModelConverter;
5+
import io.swagger.v3.core.converter.ModelConverterContext;
6+
import io.swagger.v3.core.converter.ModelConverterContextImpl;
7+
import io.swagger.v3.oas.models.media.Schema;
8+
import org.testng.annotations.Test;
9+
10+
import java.lang.reflect.Field;
11+
import java.util.Iterator;
12+
import java.util.Set;
13+
14+
import static org.testng.Assert.assertEquals;
15+
import static org.testng.Assert.assertNotNull;
16+
17+
public class AnnotatedTypeCachingTest {
18+
19+
@Test
20+
public void testAnnotatedTypeEqualityIgnoresContextualFields() {
21+
AnnotatedType type1 = new AnnotatedType(String.class)
22+
.propertyName("userStatus");
23+
AnnotatedType type2 = new AnnotatedType(String.class)
24+
.propertyName("city");
25+
assertEquals(type1, type2, "AnnotatedType objects with different contextual fields (e.g., propertyName) should be equal.");
26+
assertEquals(type1.hashCode(), type2.hashCode(), "The hash codes of equal AnnotatedType objects must be the same.");
27+
}
28+
29+
static class User {
30+
public String username;
31+
public String email;
32+
public Address address;
33+
}
34+
35+
static class Address {
36+
public String street;
37+
public String city;
38+
}
39+
40+
private static class DummyModelConverter implements ModelConverter {
41+
@Override
42+
public Schema resolve(AnnotatedType type, ModelConverterContext context, Iterator<ModelConverter> chain) {
43+
if (type.getType().equals(User.class)) {
44+
context.resolve(new AnnotatedType(String.class).propertyName("username"));
45+
context.resolve(new AnnotatedType(String.class).propertyName("email"));
46+
context.resolve(new AnnotatedType(Address.class).propertyName("address"));
47+
return new Schema();
48+
}
49+
if (type.getType().equals(Address.class)) {
50+
context.resolve(new AnnotatedType(String.class).propertyName("street"));
51+
context.resolve(new AnnotatedType(String.class).propertyName("city"));
52+
return new Schema();
53+
}
54+
return new Schema();
55+
}
56+
}
57+
58+
@Test
59+
@SuppressWarnings("unchecked")
60+
public void testCacheHitsForRepeatedStringTypeWithCorrectedEquals() throws Exception {
61+
ModelConverterContextImpl context = new ModelConverterContextImpl(new DummyModelConverter());
62+
Schema userSchema = context.resolve(new AnnotatedType(User.class));
63+
assertNotNull(userSchema);
64+
Field processedTypesField = ModelConverterContextImpl.class.getDeclaredField("processedTypes");
65+
processedTypesField.setAccessible(true);
66+
Set<AnnotatedType> processedTypes = (Set<AnnotatedType>) processedTypesField.get(context);
67+
long stringTypeCount = processedTypes.stream()
68+
.filter(annotatedType -> annotatedType.getType().equals(String.class))
69+
.count();
70+
assertEquals(stringTypeCount, 1, "With the correct equals/hashCode, String type should be added to the cache only once.");
71+
}
72+
}
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
package io.swagger.v3.core.converting;
2+
3+
import io.swagger.v3.core.converter.AnnotatedType;
4+
import org.testng.annotations.Test;
5+
6+
import java.lang.annotation.Annotation;
7+
import java.lang.annotation.ElementType;
8+
import java.lang.annotation.Retention;
9+
import java.lang.annotation.RetentionPolicy;
10+
import java.lang.annotation.Target;
11+
import java.lang.reflect.Type;
12+
import java.util.HashSet;
13+
import java.util.Set;
14+
15+
import static org.testng.Assert.assertEquals;
16+
import static org.testng.Assert.assertNotEquals;
17+
import static org.testng.Assert.assertTrue;
18+
19+
public class AnnotatedTypeTest {
20+
21+
@Retention(RetentionPolicy.RUNTIME)
22+
@Target(ElementType.TYPE)
23+
@interface TestAnnA {}
24+
25+
@Retention(RetentionPolicy.RUNTIME)
26+
@Target(ElementType.TYPE)
27+
@interface TestAnnB {}
28+
29+
@TestAnnA
30+
@TestAnnB
31+
@Deprecated
32+
private static class AnnotationHolder {}
33+
34+
private Annotation getAnnotationInstance(Class<? extends Annotation> clazz) {
35+
return AnnotationHolder.class.getAnnotation(clazz);
36+
}
37+
38+
/**
39+
* Tests that equals() and hashCode() are order-insensitive for context annotations.
40+
*/
41+
@Test
42+
public void testEqualsAndHashCode_shouldBeOrderInsensitiveForAnnotations() {
43+
Annotation annA = getAnnotationInstance(TestAnnA.class);
44+
Annotation annB = getAnnotationInstance(TestAnnB.class);
45+
AnnotatedType type1 = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annA, annB});
46+
AnnotatedType type2 = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annB, annA});
47+
assertEquals(type1, type2, "Objects should be equal even if annotation order is different.");
48+
assertEquals(type1.hashCode(), type2.hashCode(), "Hash codes should be equal even if annotation order is different.");
49+
}
50+
51+
/**
52+
* Tests that JDK/internal annotations are filtered out for equals() and hashCode() comparison.
53+
*/
54+
@Test
55+
public void testEqualsAndHashCode_shouldIgnoreJdkInternalAnnotations() {
56+
Annotation annA = getAnnotationInstance(TestAnnA.class);
57+
Annotation deprecated = getAnnotationInstance(Deprecated.class);
58+
AnnotatedType typeWithUserAnn = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annA});
59+
AnnotatedType typeWithJdkAnn = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annA, deprecated});
60+
AnnotatedType typeWithOnlyJdkAnn = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{deprecated});
61+
AnnotatedType typeWithNoAnn = new AnnotatedType(String.class);
62+
assertEquals(typeWithUserAnn, typeWithJdkAnn, "JDK annotations should be ignored in equality comparison.");
63+
assertEquals(typeWithUserAnn.hashCode(), typeWithJdkAnn.hashCode(), "JDK annotations should be ignored in hashCode calculation.");
64+
assertEquals(typeWithOnlyJdkAnn, typeWithNoAnn, "An object with only JDK annotations should be equal to one with no annotations.");
65+
assertEquals(typeWithOnlyJdkAnn.hashCode(), typeWithNoAnn.hashCode(), "The hash code of an object with only JDK annotations should be the same as one with no annotations.");
66+
}
67+
68+
/**
69+
* Tests that defensive copying prevents Set corruption from external array mutation.
70+
*/
71+
@Test
72+
public void testImmutability_shouldPreventCorruptionInHashSet() {
73+
Annotation annA = getAnnotationInstance(TestAnnA.class);
74+
Annotation annB = getAnnotationInstance(TestAnnB.class);
75+
Annotation[] originalAnnotations = new Annotation[]{annA};
76+
AnnotatedType type = new AnnotatedType(String.class).ctxAnnotations(originalAnnotations);
77+
Set<AnnotatedType> typeSet = new HashSet<>();
78+
typeSet.add(type);
79+
int initialHashCode = type.hashCode();
80+
originalAnnotations[0] = annB;
81+
assertEquals(initialHashCode, type.hashCode(), "Hash code must remain the same after mutating the external array.");
82+
assertTrue(typeSet.contains(type), "The Set must still contain the object after mutating the external array.");
83+
}
84+
85+
/**
86+
* Tests that an instance of a subclass can be equal to an instance of the parent class.
87+
*/
88+
@Test
89+
public void testEqualsAndHashCode_shouldAllowSubclassEquality() {
90+
class SubAnnotatedType extends AnnotatedType {
91+
public SubAnnotatedType(Type type) { super(type); }
92+
}
93+
Annotation annA = getAnnotationInstance(TestAnnA.class);
94+
Annotation[] annotations = {annA};
95+
AnnotatedType parent = new AnnotatedType(Integer.class).ctxAnnotations(annotations).name("number");
96+
SubAnnotatedType child = new SubAnnotatedType(Integer.class);
97+
child.ctxAnnotations(annotations);
98+
child.name("number");
99+
AnnotatedType differentParent = new AnnotatedType(Long.class).name("number");
100+
assertEquals(parent, child, "Parent and child objects should be equal if their properties are the same.");
101+
assertEquals(child, parent, "Equality comparison should be symmetric.");
102+
assertEquals(parent.hashCode(), child.hashCode(), "Parent and child hash codes should be equal if their properties are the same.");
103+
assertNotEquals(parent, differentParent, "Objects with different properties should not be equal.");
104+
}
105+
}

0 commit comments

Comments
 (0)