@NotNull이 Needs Work가 필요한 수준인가요?

코드 리뷰를 하다가 제목과 같은 질문을 받았다.

@NotNull이 Needs Work가 필요한 수준인가요?

 

사업자등록번호에 대한 유효성 제약 수정이 있어서 PR을 받았는데 의견을 남기고 추가적인 작업이 필요할 것 같아서 Needs Work 로 마크를 했었다.

 

아래와 같은 요청 파라미터로 사용되는 객체이다. (실무 코드는 아니고 예제이다.)

import lombok.Getter;
import lombok.Setter;

import javax.validation.constraints.NotBlank;
import javax.validation.constraints.NotNull;

@Getter
@Setter
public class MemberJoinRequest implements Member {
    @NotNull
    private Long id;
    @NotBlank
    private String name;
    @NotNull
    private String businessNumber;

}

나는 businessNumber 변수에 아래와 같은 comment 를 달았다.

사업자등록번호는 형태가 고정되어 있는 형태라서 request 단계에서 엄격하게 validation 을 하기에 적합해 보이네요.애초에 입력에서 엄격하게 처리를 하면 뒷 부분에서 방어로직이나 변환하는 과정이 불필요해질 수 있고결과적으로 코드가 간결해질 수 있을 것으로 생각합니다.
따라서 @NotNull 보다는 @NotBlank 가, @NotBlank 보다는 구체적인 입력 형태가 좋겠어요.

그랬더니 리뷰를 받은 사람은 아래와 같이 코드를 바꾸었다.

import lombok.Getter;
import lombok.Setter;

import javax.validation.constraints.NotBlank;
import javax.validation.constraints.NotNull;

@Getter
@Setter
public class MemberJoinRequest implements Member {
    @NotNull
    private Long id;
    @NotBlank
    private String name;
    @Length(min = 10, max = 12)
    private String businessNumber;

}

 

배경설명을 추가적으로 하면 팀 내부에서 대시 없이 숫자만으로 데이터를 저장하자는 의견에 동의해서 변화하는 과정이 있었다.

아마도 과도기 적인 과정 때문에 10자에서 12자까지 길이의 허용하는 것으로 변경을 했다.

 

하지만 여기에 맹점이 있었는데 바로 null 에 대한 제약조건이 빠지게 된다는 것이다.

테스트를 수행해보면 쉽게 알 수 있다.

import spock.lang.Shared
import spock.lang.Specification
import spock.lang.Subject

import javax.validation.ConstraintViolation
import javax.validation.Validation
import javax.validation.Validator
import javax.validation.ValidatorFactory

class MemberJoinRequestSpec extends Specification {

    @Subject
    private MemberJoinRequest sut

    @Shared
    private Validator validator

    def setupSpec() {
        ValidatorFactory factory = Validation.buildDefaultValidatorFactory()
        validator = factory.getValidator()
    }

    def setup() {
        sut = getNoConstraintValidationMemberJoinRequest()
    }

    def "사업자 등록번호는 null 을 허용하지 않는다."() {
        given:
        sut.setBusinessNumber(null)

        when:
        Set<ConstraintViolation<MemberJoinRequest>> constraintViolations = validator.validate(sut)

        then:
        constraintViolations.size() == 1
        with(constraintViolations[0]) {
            propertyPath.toString() == 'businessNumber'
            invalidValue == null
            messageTemplate == '{javax.validation.constraints.NotNull.message}'
        }
    }

    static def getNoConstraintValidationMemberJoinRequest() {
        def req = new MemberJoinRequest()
        req.id = 1
        req.name = "테스터"
        req.businessNumber = "1234567890"
        return req
    }

}

이전: @NotNull

변경후: @Length(min = 10, max = 12)

왜냐하면 Length Validator 는 null 인 경우에 true 를 반환하기 때문이다.

public class LengthValidator implements ConstraintValidator<Length, CharSequence> {
	// ..
	@Override
	public boolean isValid(CharSequence value, ConstraintValidatorContext constraintValidatorContext) {
		if ( value == null ) {
			return true;
		}
		int length = value.length();
		return length >= min && length <= max;
	}

따라서 @Length 를 붙였다고 @NotNull 를 지우면 안된다.

 

Length 에 대한 비즈니스 규칙이 추가되었으니 아래와 같은 길이에 대한 테스트를 추가할 수 있다.

@Unroll
def "사업자 등록번호가 #testBusinessNumber.length() 자리이면 길이 제약조건 위반이 발생한다."() {
    given:
    sut.setBusinessNumber(testBusinessNumber)

    when:
    Set<ConstraintViolation<MemberJoinRequest>> constraintViolations = validator.validate(sut)

    then:
    constraintViolations.size() == 1
    with(constraintViolations[0]) {
        propertyPath.toString() == 'businessNumber'
        invalidValue.toString() == testBusinessNumber
        messageTemplate == '{org.hibernate.validator.constraints.Length.message}'
    }

    where:
    testBusinessNumber << ['', '1', '123456789', '1234567890123', '12345678901234']
}

null 까지 포함해서 전체 테스트를 돌려보면 성공한다.

문제는 다음과 같은 테스트도 성공한다는 것에 있다.

def "사업자 등록번호는 문자열을 허용한다."() {
    given:
    sut.setBusinessNumber('tablelands')

    when:
    Set<ConstraintViolation<MemberJoinRequest>> constraintViolations = validator.validate(sut)

    then:
    noConstraintViolation(constraintViolations)
}

@Unroll
def "사업자 등록번호가 #testBusinessNumber.length() 자리를 허용한다."() {
    given:
    sut.setBusinessNumber(testBusinessNumber)

    when:
    Set<ConstraintViolation<MemberJoinRequest>> constraintViolations = validator.validate(sut)

    then:
    noConstraintViolation(constraintViolations)

    where:
    testBusinessNumber = '12345678901'
}

사업자등록번호는 10자리뿐인데 10자리와 12 자리 사이의 11자리를 허용하게 되고, 심지어 문자열을 넣어도 validation 에 통과하게 된다는 것이다.

 

물론 사람에 따라서 입력 파라미터에 대해 관대하게 프로그래밍을 할 수도 있을 것이다.

하지만 이러한 관대함이 나중에는 예상하지 못한 버그와 문제를 만드는 것을 경험으로 배웠다.

그리고 QA 를 해보면 이런 입력 파라미터에 대해 높은 확률로 지적을 당한다. 그들은 이런 경우의 수를 만드는 것에 도가 떴기 때문이다.

 

클린 코더 1장 프로의 마음가짐에 보면 이런 이야기가 나온다.

QA는 아무것도 찾지 못해야 한다

QA가 문제를 찾지 못할 것이라고 어느 정도 자신할 수 있어야 한다.
코드에 결함이 있는 걸 알면서도 QA에게 코드를 보내는 일은 매우 프로답지 못한 행동이다.
어떤 이들은 QA를 오류를 찾는 용도로 사용한다.
QA 혹은 사용자가 문제를 찾을 때마다, 개발자는 놀라움과 분함을 느껴야 마땅하다.
-프로의 마음가짐(p.51)-

그렇다면 나라면 어떻게 유효성 검사를 구성했을까?

하이버네이터 제약 조건 중에 하나인 @CreditCardNumber 이나 @EAN 처럼 @BusinessNumber 라는 커스텀 validator를 만들었을 수도 있다. 어떤 자리에 어떤 숫자까지 와야하는지 까지 말이다. 정말이지 과하게 했다면 국세청 사업자등록상태조회에서 네트워크를 통한 검사를 할 수도 있겠다.

하지만 자주 빠르게 사용하는 목적으로 한다면 아래와 같이 정규식으로 구현했을 수도 있겠다.

public class MemberJoinRequest implements Member {
    @NotNull
    private Long id;
    @NotBlank
    private String name;
    @NotNull
    @Pattern(regexp = "\\d{10}|\\d{3}-\\d{2}-\\d{5}", message = "10자리의 숫자 또는 대시를 포함한 12자리의 숫자만 입력가능합니다.")
    private String businessNumber;

}

테스트도 가능한 데이터와 불가능한 몇 개를 테스트 하면 되지 않을까 싶다.

관련된 코드는 깃헙에서 확인이 가능하다.

https://github.com/namhokim/studySpring/pull/2/commits