새 API 컨트롤러를 리뷰하다가 수정 잠금 검사가 통째로 주석 처리돼 있는 것을 봤다. 지우지 않고 주석으로 남긴 형태라 의도적으로 끈 것으로 보였다.
Table of contents
Open Table of contents
상위는 꺼지고 하위는 켜져 있었다
이 플래그를 쓰는 곳을 전부 찾으니 서비스 계층과 작업 핸들러와 관리자 컨트롤러와 이전 버전 API 컨트롤러 네 곳이었다. 그중 작업 핸들러를 열어 보니 같은 검사가 살아 있어서 잠금이 걸린 상품이면 작업 생성을 막고 있었다.
if ($product->is_modify_lock) {
return false; // 잡 생성 차단
}그러면 흐름이 이렇게 된다. API 요청이 컨트롤러의 꺼진 검사를 통과해서 수정이 DB에 반영되고, 그다음 전송 작업을 만드는 자리에서 켜져 있는 검사에 걸려 차단된다. 우리 DB는 바뀌었는데 바깥으로는 안 나간 상태가 된다.
조용한 불일치
이 조합이 나쁜 것은 어느 쪽도 오류로 나타나지 않기 때문이다. API는 성공을 돌려주고 우리 DB에는 값이 반영돼 있으니 호출한 쪽에서는 정상 처리된 것으로 읽힌다. 바깥 값이 안 바뀌었다는 것은 한참 뒤에 값을 대조하는 자리에서야 드러난다.
검사가 양쪽 다 켜져 있었으면 요청 단계에서 거부됐을 것이고, 양쪽 다 꺼져 있었으면 끝까지 반영됐을 것이다. 반쪽만 꺼진 상태가 가장 조용하게 어긋난다.
의도와 확인의 구분
이 주석 처리가 실수인지 판단인지를 보려고 이전 버전 컨트롤러를 열었더니 같은 자리가 같은 방식으로 주석 처리돼 있었다. 두 곳이 같은 모양이면 손이 미끄러진 것으로 보기는 어렵다.
다만 기록할 때는 확인한 것과 추정한 것을 나눠 적었다. 상위가 주석 처리돼 있고 하위가 활성이며 그 결과 불일치가 생길 수 있다는 것까지는 코드로 확인한 사실이고, 그것이 의도적 판단이라는 것은 두 버전이 같다는 데서 나온 추정이다. 코드가 같다는 것과 의도가 같다는 것은 다른 얘기이고 한쪽을 복사해서 만들었을 수도 있다.
의도가 맞다고 해도 지금 상태는 그 의도대로 동작하지 않는다. API에서 잠금을 무시하고 수정을 허용하려던 것이라면 하위도 같이 꺼야 수정이 실제로 반영된다.
나머지 둘의 성격
같은 리뷰에서 두 가지가 더 나왔다. 하나는 예외 처리 블록이 통째로 주석 처리된 것이다. 예외가 나면 프레임워크 기본 오류 응답이 나가면서 스택 트레이스와 파일 경로와 쿼리가 호출한 쪽에 그대로 전달된다. 디버깅하려고 잠시 풀어 둔 것을 되돌리지 않은 형태로 보인다.
다른 하나는 삼항 조건이 뒤집힌 것이다. 채널 식별자가 있으면 생성이고 없으면 갱신으로 돼 있는데 기준 구현은 반대였다. 논리로 따져도 식별자가 있다는 것은 이미 등록됐다는 뜻이니 갱신이 맞고 새 코드 쪽이 뒤집혀 있었다.
셋을 나란히 놓으면 원인이 다르다. 첫째는 판단은 있었으나 적용 범위가 부족한 것이고, 둘째는 임시 조치를 안 되돌린 실수이며, 셋째는 논리 오류다.
주석 처리와 사용처 전수
앞의 둘에는 공통점이 있다. 둘 다 삭제가 아니라 주석 처리로 남아 있었다. 코드를 지우면 diff에 삭제로 나타나 리뷰에서 눈에 띄는데, 주석으로 남기면 그 자리에 코드가 있으므로 훑어볼 때 살아 있는 것처럼 보인다. 주석 처리된 코드는 있는 것도 없는 것도 아닌 상태로 남는다.
첫 번째를 잡아 준 것은 사용처 전수 조사였다. 컨트롤러만 보면 검사가 꺼져 있다는 데서 판단이 끝나는데, 그 플래그를 쓰는 자리를 전부 열어 봤기 때문에 하위가 켜져 있다는 것이 나왔다. 어떤 검사를 끄기로 했으면 그 값을 읽는 다른 자리도 같이 봐야 한다는 것이 이 건에서 남은 것이다.
정리
- 같은 플래그가 상위에서는 꺼지고 하위에서는 켜져 있을 수 있다
- 그러면 우리 DB는 바뀌고 바깥으로는 안 나가는 불일치가 생긴다
- 응답이 성공이라 호출한 쪽에서는 정상 처리로 읽힌다
- 반쪽만 꺼진 상태가 양쪽 다 꺼진 것보다 조용하게 어긋난다
- 두 곳이 같은 모양이면 의도로 추정되지만 확인한 사실과는 나눠 적는다
- 끄기로 했으면 그 값을 읽는 자리를 전부 꺼야 의도대로 동작한다
- 주석 처리된 코드는 훑어볼 때 살아 있는 것처럼 보인다
- 예외 처리를 풀어 두면 스택 트레이스와 경로가 그대로 나간다
- 검사를 끌 때는 그 값을 읽는 사용처를 전수로 본다