Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor : Adjust resolveFieldName Method Logic to Correctly Handle Primitive boolean Field Names #1096

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rawfishthelgh
Copy link

Summary

JavaGetterPropertyFieldNameResolverresolveFieldName 메소드가 필드 값을 추출하는 과정에서,
원시 타입 boolean 필드값 추출 시 올바른 필드명이 추출되지 않아 메소드 순서를 변경했습니다.

Description

Lombok@Getter 어노테이션을 사용할 때, boolean 타입을 primitive type으로 설정하고, 필드명의 prefix를 “isXXX” 로 사용하는 경우 문제가 있습니다.

@Entity
@NoArgsConstructor
@AllArgsConstructor
@Builder
@Getter
public class Sample {

	@Id @GeneratedValue(strategy = GenerationType.IDENTITY)
	private Long id;

	private boolean isUsed;

}

위와 같은 엔티티에서 boolean isUsed; 라는 필드가 있다고 가정할 때,

@Test
	void test(){
		FixtureMonkey fixtureMonkey = FixtureMonkey.builder()
			.plugin(new JakartaValidationPlugin()) 
			.objectIntrospector(BuilderArbitraryIntrospector.INSTANCE)
			.build();

		Sample sample = fixtureMonkey.giveMeBuilder(Sample.class)
			.set(javaGetter(Sample::isUsed), true)
			.sample();
	}

Lombok 은 위처럼 필드명과 동일한 getter 를 생성합니다. ( Sample::isUsed )
스크린샷 2024-11-19 오후 8 58 39
하지만 JavaGetterPropertyFieldNameResolverresolveFieldName 메소드에서,
else if (hasPrefix(IS_PREFIX, methodName))true 일 경우

스크린샷 2024-11-19 오후 8 59 06 스크린샷 2024-11-19 오후 8 59 56

stripPrefixPropertyName(targetClass, methodName, IS_PREFIX.length()) 를 수행하기 때문에, isUsed 필드명이 used 로 strip되어 resolve되기에 NPE 에러가 발생합니다.

//JavaGetterPropertyFieldNameResolver.java
@Nullable
	public String resolveFieldName(Class<?> targetClass, String methodName) {

		if (isValidField(targetClass, methodName)) {
			return methodName;
		} else if (hasPrefix(GET_PREFIX, methodName)) {
			return stripPrefixPropertyName(targetClass, methodName, GET_PREFIX.length());
		} else if (hasPrefix(IS_PREFIX, methodName)) {
			return stripPrefixPropertyName(targetClass, methodName, IS_PREFIX.length());
		} 

		return null;
	}

위처럼 isValidField(targetClass, methodName) 검증을 가장 먼저 수행하도록 변경하면, 필드명과 메소드명이 동일한 경우에도 대응이 가능해 변경했습니다.

How Has This Been Tested?

Passed All Existing Tests

@CLAassistant
Copy link

CLAassistant commented Nov 19, 2024

CLA assistant check
All committers have signed the CLA.

@@ -28,13 +28,14 @@ public final class JavaGetterPropertyFieldNameResolver {

@Nullable
public String resolveFieldName(Class<?> targetClass, String methodName) {
if (hasPrefix(GET_PREFIX, methodName)) {

if (isValidField(targetClass, methodName)) {
Copy link
Contributor

@seongahjo seongahjo Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하위호환을 보장하고 안전하게 변경됨을 보장하기 위해 다음 테스트들이 추가되면 좋을 것 같습니다.

  1. is가 이름의 prefix로 붙은 boolean이 아닌 필드를 set
  2. is가 이름의 prefix로 붙지않은 boolean의 필드를 set
  3. is가 이름의 prefix로 붙은 boolean인 필드를 set
  4. is가 이름의 prefix로 붙지않은 boolean이 아닌 필드를 set

Copy link
Author

@rawfishthelgh rawfishthelgh Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 추가했습니다!

  1. is가 이름의 prefix로 붙은 boolean이 아닌 필드를 set
  2. is가 이름의 prefix로 붙지않은 boolean의 필드를 set
  3. is가 이름의 prefix로 붙은 boolean인 필드를 set
  4. is가 이름의 prefix로 붙지않은 boolean이 아닌 필드를 set

추가로 Boolean 필드가 Wrapper Type일 경우와 Primitive Type일 경우의 테스트도 작성했습니다.

@seongahjo seongahjo added this to the 1.1.4 milestone Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants