728x90
드디어 처음으로 PR이 Merge 됐다!
https://github.com/swagger-api/swagger-core/pull/4975
Fix(core): Prevent redundant schema resolution by fixing AnnotatedType equality by juntae6942 · Pull Request #4975 · swagger-a
Pull Request Description This pull request resolves a performance issue where the ModelConverterContext fails to cache AnnotatedType instances, leading to redundant schema resolutions. The root cau...
github.com
지난 8월에 나는 오픈소스 기여모임에 참여하면서, 처음으로 오픈소스에 기여 할 수 있는 방법을 학습했다.
항상 막연하게 기여해보고 싶다...기여...ㄱ...ㅇ..하면서 아무것도 할 수 없었던 감자에서 이제는 자신있게 이슈를 찾아서 기여하고, Merge까지 되는 성장 한 감자가 됐다.ㅎㅎ 매우 보람있고, 지금의 상황과 개발이 더욱 즐거워졌다.
한 달이라는 시간 동안 운영진분들께 많은 오픈소스 기여 문화를 배우고, 이슈를 해결하는 방법을 몸으로 깨달으면서 정말 좋은 기회를 통해서 성장 할 수 있음에 다시 한 번 운영진분들께 감사함을 전하고 싶다ㅎㅎ.
어떠한 이슈였는가?
해당 이슈는 ModelConverterContext가 AnnotatedType 인스턴스를 제대로 캐싱하지 못해, 불필요하게 스키마를 반복적으로 처리하는 성능 문제였다.
해당 이슈에 대해서 이슈를 올린 개발자분께서 상황을 잘 설명해주셨고, 원인이 되는 AnnotateType의 equals()와 hashcode() 부분을 언급해주셔서 조금 더 쉽게 이 문제를 해결 할 수 있었다. 또한 메인테이너께서 이 이슈를 직접 나에게 Assign 해주셨다. 그래서 먼저 문제가 되는 equals와 hashcode의 조건문을 뜯어보았다. 기존의 코드에서는 type과 ctxAnnotations만을 가지고 같은 AnnotateType인지 비교하였었는데, 이런 경우 내용은 다르지만, type과 ctxAnnotations이 같은 AnnotateType이 모두 동일한 객체로 인식하여서 캐시가 잘못된 정보를 반환하여 의도와 다르게 ModelConverterContext의 resolve 메서드에서 동일한 AnnotatedType이 매번 다시 resolve 처리되는 버그로 판단했다.
어떻게 문제를 해결하였는가?
문제를 해결하기 위해서 equals의 제한을 좀 더 엄격하게 하고, 이에 따른 hashcode도 재구성했다.
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
AnnotatedType that = (AnnotatedType) o;
return skipOverride == that.skipOverride &&
schemaProperty == that.schemaProperty &&
resolveAsRef == that.resolveAsRef &&
resolveEnumAsRef == that.resolveEnumAsRef &&
includePropertiesWithoutJSONView == that.includePropertiesWithoutJSONView &&
skipSchemaName == that.skipSchemaName &&
skipJsonIdentity == that.skipJsonIdentity &&
Objects.equals(type, that.type) &&
Objects.equals(name, that.name) &&
Arrays.equals(ctxAnnotations, that.ctxAnnotations) &&
Objects.equals(jsonViewAnnotation, that.jsonViewAnnotation);
}
@Override
public int hashCode() {
int result = Objects.hash(type, name, skipOverride, schemaProperty, resolveAsRef,
resolveEnumAsRef, jsonViewAnnotation, includePropertiesWithoutJSONView, skipSchemaName,
skipJsonIdentity);
result = 31 * result + Arrays.hashCode(ctxAnnotations);메인테이너분께서 아래와 같은 피드백을 주셨다.
- 어노테이션 순서 문제: 현재 equals는 배열 순서까지 비교하기 때문에, 내용이 같아도 순서만 다른 어노테이션({@A, @B} vs {@B, @A})을 다른 것으로 판단합니다. 이로 인해 불필요한 캐시 미스가 발생할 수 있으니, 순서에 무관하게 비교하도록 개선이 필요합니다.
- JDK 내부 어노테이션 처리 불일치: hashCode 메소드는 jdk.나 sun. 같은 내부 어노테이션을 무시하도록 구현되었지만, equals 메소드는 여전히 그대로 비교하고 있습니다. 이 두 메소드의 동작을 일치시켜야 합니다.
- 가변성(Mutability) 위험: equals 비교에 사용되는 ctxAnnotations 필드가 변경 가능한 배열(array)이라서 문제입니다. 만약 객체를 Set이나 Map에 넣은 후 이 배열의 내용이 바뀌면, 컬렉션이 객체를 제대로 찾지 못하는 심각한 오류가 발생할 수 있습니다.
- 상속 클래스와의 동등성 문제: instanceof 대신 getClass()로 비교 로직을 바꾸면 하위 클래스와의 동등성 비교가 깨지는 동작 변경이 발생할 수 있습니다. 현재로서는 instanceof를 유지하는 것이 좋다고 제안합니다.
메인테이너분의 피드백에 따라서 추가로 어노테이션 배열을 입력받아 의미 있는 어노테이션만 걸러내고, 일정한 순서로 정렬하여 새로운 리스트(List)로 반환하여, java., jdk., sun. 패키지에 속한 표준 자바 어노테이션들을 모두 제거하고, 이를 통해 사용자가 직접 정의했거나 라이브러리에서 추가한 핵심적인 어노테이션만 남겨서 어노테이션이 어떤 순서로 선언되었든 상관없이 항상 동일한 순서와 내용을 가진 결과 리스트를 얻을 수 있게 만드는 getProcessedAnnotations을 만들었다.
private List<Annotation> getProcessedAnnotations(Annotation[] annotations) {
if (annotations == null || annotations.length == 0) {
return new ArrayList<>();
}
return Arrays.stream(annotations)
.filter(a -> {
String pkg = a.annotationType().getPackage().getName();
return !pkg.startsWith("java.") && !pkg.startsWith("jdk.") && !pkg.startsWith("sun.");
})
.sorted(Comparator.comparing(a -> a.annotationType().getName()))
.collect(Collectors.toList());
}이를 통해서 equals나 hashCode 메소드에서 객체의 동등성을 안정적으로 비교했다.
그 다음으로는 너무 엄격하게 조건을 주다보니 불필요한 조건들까지 들어간 equals를 단순화하는 작업과 메인테이너분이 피드백 주신 instanceof 사용 부분의 수정을 다시 진행했다.
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof AnnotatedType)) return false;
AnnotatedType that = (AnnotatedType) o;
List<Annotation> thisAnnotatinons = getProcessedAnnotations(this.ctxAnnotations);
List<Annotation> thatAnnotatinons = getProcessedAnnotations(that.ctxAnnotations);
return includePropertiesWithoutJSONView == that.includePropertiesWithoutJSONView &&
Objects.equals(type, that.type) &&
Objects.equals(thisAnnotatinons, thatAnnotatinons) &&
Objects.equals(jsonViewAnnotation, that.jsonViewAnnotation);
}
@Override
public int hashCode() {
List<Annotation> processedAnnotations = getProcessedAnnotations(this.ctxAnnotations);
return Objects.hash(type, jsonViewAnnotation, includePropertiesWithoutJSONView, processedAnnotations);
}이를 통해서 최종 수정된 코드가 작성되었고, 이에 따른 test code 또한 작성했다.
AnnotatedType
package io.swagger.v3.core.converter;
import com.fasterxml.jackson.annotation.JsonView;
import io.swagger.v3.oas.models.Components;
import io.swagger.v3.oas.models.media.Schema;
import java.lang.annotation.Annotation;
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
import java.util.stream.Collectors;
public class AnnotatedType {
-- 기존 유지되는 전체 코드 생략 --
public Annotation[] getCtxAnnotations() {
return ctxAnnotations == null ? null : Arrays.copyOf(ctxAnnotations, ctxAnnotations.length);
}
public void setCtxAnnotations(Annotation[] ctxAnnotations) {
this.ctxAnnotations = ctxAnnotations == null ? null : Arrays.copyOf(ctxAnnotations, ctxAnnotations.length);
}
public AnnotatedType ctxAnnotations(Annotation[] ctxAnnotations) {
setCtxAnnotations(ctxAnnotations);
return this;
}
private List<Annotation> getProcessedAnnotations(Annotation[] annotations) {
if (annotations == null || annotations.length == 0) {
return new ArrayList<>();
}
return Arrays.stream(annotations)
.filter(a -> {
String pkg = a.annotationType().getPackage().getName();
return !pkg.startsWith("java.") && !pkg.startsWith("jdk.") && !pkg.startsWith("sun.");
})
.sorted(Comparator.comparing(a -> a.annotationType().getName()))
.collect(Collectors.toList());
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof AnnotatedType)) return false;
AnnotatedType that = (AnnotatedType) o;
List<Annotation> thisAnnotatinons = getProcessedAnnotations(this.ctxAnnotations);
List<Annotation> thatAnnotatinons = getProcessedAnnotations(that.ctxAnnotations);
return includePropertiesWithoutJSONView == that.includePropertiesWithoutJSONView &&
Objects.equals(type, that.type) &&
Objects.equals(thisAnnotatinons, thatAnnotatinons) &&
Objects.equals(jsonViewAnnotation, that.jsonViewAnnotation);
}
@Override
public int hashCode() {
List<Annotation> processedAnnotations = getProcessedAnnotations(this.ctxAnnotations);
return Objects.hash(type, jsonViewAnnotation, includePropertiesWithoutJSONView, processedAnnotations);
}
}
AnnotatedTypeTest
package io.swagger.v3.core.converting;
import io.swagger.v3.core.converter.AnnotatedType;
import org.testng.annotations.Test;
import java.lang.annotation.Annotation;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Type;
import java.util.HashSet;
import java.util.Set;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotEquals;
import static org.testng.Assert.assertTrue;
public class AnnotatedTypeTest {
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@interface TestAnnA {}
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@interface TestAnnB {}
@TestAnnA
@TestAnnB
@Deprecated
private static class AnnotationHolder {}
private Annotation getAnnotationInstance(Class<? extends Annotation> clazz) {
return AnnotationHolder.class.getAnnotation(clazz);
}
/**
* Tests that equals() and hashCode() are order-insensitive for context annotations.
*/
@Test
public void testEqualsAndHashCode_shouldBeOrderInsensitiveForAnnotations() {
Annotation annA = getAnnotationInstance(TestAnnA.class);
Annotation annB = getAnnotationInstance(TestAnnB.class);
AnnotatedType type1 = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annA, annB});
AnnotatedType type2 = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annB, annA});
assertEquals(type1, type2, "Objects should be equal even if annotation order is different.");
assertEquals(type1.hashCode(), type2.hashCode(), "Hash codes should be equal even if annotation order is different.");
}
/**
* Tests that JDK/internal annotations are filtered out for equals() and hashCode() comparison.
*/
@Test
public void testEqualsAndHashCode_shouldIgnoreJdkInternalAnnotations() {
Annotation annA = getAnnotationInstance(TestAnnA.class);
Annotation deprecated = getAnnotationInstance(Deprecated.class);
AnnotatedType typeWithUserAnn = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annA});
AnnotatedType typeWithJdkAnn = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{annA, deprecated});
AnnotatedType typeWithOnlyJdkAnn = new AnnotatedType(String.class).ctxAnnotations(new Annotation[]{deprecated});
AnnotatedType typeWithNoAnn = new AnnotatedType(String.class);
assertEquals(typeWithUserAnn, typeWithJdkAnn, "JDK annotations should be ignored in equality comparison.");
assertEquals(typeWithUserAnn.hashCode(), typeWithJdkAnn.hashCode(), "JDK annotations should be ignored in hashCode calculation.");
assertEquals(typeWithOnlyJdkAnn, typeWithNoAnn, "An object with only JDK annotations should be equal to one with no annotations.");
assertEquals(typeWithOnlyJdkAnn.hashCode(), typeWithNoAnn.hashCode(), "The hash code of an object with only JDK annotations should be the same as one with no annotations.");
}
/**
* Tests that defensive copying prevents Set corruption from external array mutation.
*/
@Test
public void testImmutability_shouldPreventCorruptionInHashSet() {
Annotation annA = getAnnotationInstance(TestAnnA.class);
Annotation annB = getAnnotationInstance(TestAnnB.class);
Annotation[] originalAnnotations = new Annotation[]{annA};
AnnotatedType type = new AnnotatedType(String.class).ctxAnnotations(originalAnnotations);
Set<AnnotatedType> typeSet = new HashSet<>();
typeSet.add(type);
int initialHashCode = type.hashCode();
originalAnnotations[0] = annB;
assertEquals(initialHashCode, type.hashCode(), "Hash code must remain the same after mutating the external array.");
assertTrue(typeSet.contains(type), "The Set must still contain the object after mutating the external array.");
}
/**
* Tests that an instance of a subclass can be equal to an instance of the parent class.
*/
@Test
public void testEqualsAndHashCode_shouldAllowSubclassEquality() {
class SubAnnotatedType extends AnnotatedType {
public SubAnnotatedType(Type type) { super(type); }
}
Annotation annA = getAnnotationInstance(TestAnnA.class);
Annotation[] annotations = {annA};
AnnotatedType parent = new AnnotatedType(Integer.class).ctxAnnotations(annotations).name("number");
SubAnnotatedType child = new SubAnnotatedType(Integer.class);
child.ctxAnnotations(annotations);
child.name("number");
AnnotatedType differentParent = new AnnotatedType(Long.class).name("number");
assertEquals(parent, child, "Parent and child objects should be equal if their properties are the same.");
assertEquals(child, parent, "Equality comparison should be symmetric.");
assertEquals(parent.hashCode(), child.hashCode(), "Parent and child hash codes should be equal if their properties are the same.");
assertNotEquals(parent, differentParent, "Objects with different properties should not be equal.");
}
}
- 어노테이션의 순서가 달라도 내용이 같으면 동일한 객체로 취급해야 한다.
- 비교에 불필요한 JDK 기본 어노테이션은 무시해야 한다.
- 외부에서 객체의 내부 상태를 변경할 수 없도록 불변성이 보장되어야 한다.
- 하위 클래스 객체라도 속성이 같다면 상위 클래스 객체와 동일하다고 판단해야 한다.
1. 어노테이션 순서에 무관한 동등성 테스트 (testEqualsAndHashCode_shouldBeOrderInsensitiveForAnnotations)
- 테스트 목적: equals와 hashCode가 어노테이션의 선언 순서에 영향을 받지 않는지 검증합니다.
- 테스트 방법:
- type1 객체에는 {@A, @B} 순서로 어노테이션을 설정합니다.
- type2 객체에는 {@B, @A} 순서로 어노테이션을 설정합니다.
- type1과 type2가 서로 equals를 통해 같다고 판단되는지, 그리고 hashCode() 결과값도 동일한지 확인합니다.
- 검증 내용: 이 테스트를 통과하면 AnnotatedType은 어노테이션의 순서가 아닌 내용을 기준으로 동등성을 비교한다는 것을 보장할 수 있습니다.
2. JDK 기본 어노테이션 무시 테스트 (testEqualsAndHashCode_shouldIgnoreJdkInternalAnnotations)
- 테스트 목적: @Deprecated와 같이 Java에 내장된 기본 어노테이션(JDK 어노테이션)들은 동등성 비교 시 무시되는지 검증합니다.
- 테스트 방법:
- 사용자 정의 어노테이션(@TestAnnA)만 가진 객체를 만듭니다.
- 사용자 정의 어노테이션과 JDK 어노테이션(@Deprecated)을 함께 가진 객체를 만듭니다.
- 두 객체가 서로 같다고 판단되는지 확인합니다.
- 검증 내용: 이 테스트는 동등성 비교가 의미 있는 어노테이션에만 집중하고, 부가적인 JDK 어노테이션 '노이즈'를 잘 걸러내는지 확인합니다.
3. 불변성(Immutability) 및 방어적 복사 테스트 (testImmutability_shouldPreventCorruptionInHashSet)
- 테스트 목적: AnnotatedType 객체 생성 시 사용된 외부 배열이 변경되더라도, 객체 내부 상태는 영향을 받지 않는지(불변성) 검증합니다.
- 테스트 방법:
- 어노테이션 배열 originalAnnotations를 만듭니다.
- 이 배열을 사용해 AnnotatedType 객체를 생성하고 HashSet에 추가합니다.
- 원본 배열 originalAnnotations의 내용을 다른 어노테이션으로 변경합니다.
- 객체의 hashCode가 변경되지 않았는지, 그리고 HashSet이 여전히 해당 객체를 잘 찾는지 확인합니다.
- 검증 내용: 이 테스트는 AnnotatedType이 생성될 때 외부 배열을 그대로 참조하는 것이 아니라, **내부적으로 복사본을 만들어 저장(방어적 복사)**하는지를 확인합니다. 이를 통해 HashSet이나 HashMap의 키가 외부 요인에 의해 손상되는 치명적인 문제를 예방할 수 있습니다.
4. 하위 클래스와의 동등성 보장 테스트 (testEqualsAndHashCode_shouldAllowSubclassEquality)
- 테스트 목적: AnnotatedType을 상속받은 하위 클래스의 객체라도, 속성값이 같다면 부모 클래스 객체와 동등하게 취급되는지 검증합니다.
- 테스트 방법:
- AnnotatedType을 상속하는 SubAnnotatedType 클래스를 정의합니다.
- 부모 클래스 객체와 자식 클래스 객체를 생성하고, 두 객체의 속성값을 모두 동일하게 설정합니다.
- 두 객체가 equals를 통해 같다고 나오는지 확인합니다.
- 검증 내용: 이 테스트는 equals 메소드가 getClass() != o.getClass()가 아닌, instanceof 키워드를 사용하여 구현되었음을 보장합니다. 이를 통해 유연한 객체지향적 동등성 비교가 가능해집니다
AnnotatedTypeCachingTest
package io.swagger.v3.core.converting;
import io.swagger.v3.core.converter.AnnotatedType;
import io.swagger.v3.core.converter.ModelConverter;
import io.swagger.v3.core.converter.ModelConverterContext;
import io.swagger.v3.core.converter.ModelConverterContextImpl;
import io.swagger.v3.oas.models.media.Schema;
import org.testng.annotations.Test;
import java.lang.reflect.Field;
import java.util.Iterator;
import java.util.Set;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
public class AnnotatedTypeCachingTest {
@Test
public void testAnnotatedTypeEqualityIgnoresContextualFields() {
AnnotatedType type1 = new AnnotatedType(String.class)
.propertyName("userStatus");
AnnotatedType type2 = new AnnotatedType(String.class)
.propertyName("city");
assertEquals(type1, type2, "AnnotatedType objects with different contextual fields (e.g., propertyName) should be equal.");
assertEquals(type1.hashCode(), type2.hashCode(), "The hash codes of equal AnnotatedType objects must be the same.");
}
static class User {
public String username;
public String email;
public Address address;
}
static class Address {
public String street;
public String city;
}
private static class DummyModelConverter implements ModelConverter {
@Override
public Schema resolve(AnnotatedType type, ModelConverterContext context, Iterator<ModelConverter> chain) {
if (type.getType().equals(User.class)) {
context.resolve(new AnnotatedType(String.class).propertyName("username"));
context.resolve(new AnnotatedType(String.class).propertyName("email"));
context.resolve(new AnnotatedType(Address.class).propertyName("address"));
return new Schema();
}
if (type.getType().equals(Address.class)) {
context.resolve(new AnnotatedType(String.class).propertyName("street"));
context.resolve(new AnnotatedType(String.class).propertyName("city"));
return new Schema();
}
return new Schema();
}
}
@Test
@SuppressWarnings("unchecked")
public void testCacheHitsForRepeatedStringTypeWithCorrectedEquals() throws Exception {
ModelConverterContextImpl context = new ModelConverterContextImpl(new DummyModelConverter());
Schema userSchema = context.resolve(new AnnotatedType(User.class));
assertNotNull(userSchema);
Field processedTypesField = ModelConverterContextImpl.class.getDeclaredField("processedTypes");
processedTypesField.setAccessible(true);
Set<AnnotatedType> processedTypes = (Set<AnnotatedType>) processedTypesField.get(context);
long stringTypeCount = processedTypes.stream()
.filter(annotatedType -> annotatedType.getType().equals(String.class))
.count();
assertEquals(stringTypeCount, 1, "With the correct equals/hashCode, String type should be added to the cache only once.");
}
}
AnnotatedType의 동등성을 비교할 때, propertyName(username, city 등)과 같이 문맥에 따라 달라지는 정보는 무시해야 동일한 타입을 중복으로 처리하는 비효율을 막을 수 있다는 것을 증명
1. 컨텍스트 필드를 무시하는 동등성 테스트 (testAnnotatedTypeEqualityIgnoresContextualFields)
- 테스트 목적: AnnotatedType 객체들이 핵심 타입(String.class)만 같다면, propertyName과 같은 부가적인 컨텍스트 정보가 달라도 서로 동일한 객체로 취급되는지를 검증합니다.
- 테스트 방법:
- String 타입을 가지지만 propertyName이 "userStatus"인 type1 객체를 생성합니다.
- 마찬가지로 String 타입을 가지지만 propertyName이 "city"인 type2 객체를 생성합니다.
- assertEquals(type1, type2)를 호출하여 두 객체가 equals 메소드를 통해 같다고 판단되는지 확인합니다.
- 검증 내용: 이 테스트는 캐시의 기본 원칙을 설정합니다. 즉, 캐시는 객체의 타입(String)이 무엇인지가 중요하지, 그 타입이 '어떤 속성명으로 사용되었는지'는 중요하지 않다는 규칙을 확인하는 것입니다.
2. 수정된 equals/hashCode를 이용한 캐시 효율성 테스트 (testCacheHitsForRepeatedStringTypeWithCorrectedEquals)
- 테스트 목적: 위에서 검증한 equals 규칙 덕분에, 복잡한 객체를 처리할 때 동일한 타입이 여러 번 등장해도 캐시에는 단 한 번만 저장되는지를 실증적으로 보여줍니다.
- 테스트 방법:
- 내부에 여러 개의 String 필드(username, email, street, city)를 가진 User와 Address 모델을 정의합니다.
- DummyModelConverter는 이 모델들을 처리하는 시뮬레이터입니다. User를 처리하는 과정에서 총 4개의 String 타입(username, email, street, city)을 각각 다른 propertyName으로 해석(resolve)하게 됩니다.
- 실제로 User 모델을 처리한 후, 내부 캐시 역할을 하는 processedTypes라는 Set에 접근합니다.
- 이 Set 안에 String 타입의 AnnotatedType 객체가 몇 개 저장되었는지 개수를 셉니다.
- 검증 내용: String 타입이 4번이나 처리되었음에도 불구하고, assertEquals(stringTypeCount, 1)를 통해 캐시에는 단 1개만 저장되었음을 확인합니다. 이는 equals 메소드가 propertyName을 올바르게 무시했기 때문에, 4개의 AnnotatedType 객체가 모두 "동일하다"고 판단되어 Set에 중복 저장되지 않았다는 의미입니다.
이렇게 test code와 equals(), hash()코드 수정을 통해서 첫 기여를 마무리 할 수 있었다.
728x90