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

group 설정 추가로 @Validated 검증 로직 보완 #669

Merged
merged 2 commits into from
Sep 22, 2024

Conversation

kyum-q
Copy link
Contributor

@kyum-q kyum-q commented Sep 19, 2024

⚡️ 관련 이슈

close #667

📍주요 변경 사항

  • @Validated 으로 검증 로직을 작동시키기 위해 dto 필드에 group 설정 추가

🎸기타

테스트 코드 추가

아직 컨트롤러 테스트 보완이 안되있어서 고민하다가 그래도 현재 fix하는 로직이 잘 작동하는지 확인하기 위해 dto 검증 테스트 코드 추가했습니다.

primitive type not null 로직

primitive type 은 null 일 수 없으니 NotNull 체크가 없어도 될 것 같은데 어떻게 생각하시나요?

Dto 단에서 추가해야하는 검증 로직

이 외에도 없는지 추후 컨트롤러 테스트 보완하면서 확인하면 좋을 것 같다

  • CreateTemplateRequest.thumbnailOrdinal 은 1부터 시작한다
  • CreateSourceCodeRequest.ordinal 은 1부터 시작한다

@kyum-q kyum-q added bug 개발자가 의도하지 않은 상황 BE 백엔드 labels Sep 19, 2024
@kyum-q kyum-q added this to the 5차 스프린트🍗 milestone Sep 19, 2024
@kyum-q kyum-q self-assigned this Sep 19, 2024
Copy link
Contributor

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

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

켬미 굿!!

추가로 @NotBlank가 NotNullGroup으로 되어있는 곳이 있고 그룹이 지정되지 않은 곳이 있더라구요.
@NotBlank도 같이 추가 부탁드려도 될까요~?

image

Copy link
Contributor

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

켬미 멋있어요~!

아직 컨트롤러 테스트 보완이 안되있어서 고민하다가 그래도 현재 fix하는 로직이 잘 작동하는지 확인하기 위해 dto 검증 테스트 코드 추가했습니다.

dto 검증 테스트 코드를 추가했다고 했는데, 본 PR의 커밋에서는 볼 수 없는건가요?

@kyum-q
Copy link
Contributor Author

kyum-q commented Sep 20, 2024

@jminkkk

추가로 @NotBlank가 NotNullGroup으로 되어있는 곳이 있고 그룹이 지정되지 않은 곳이 있더라구요.

@NotBlank도 같이 추가 부탁드려도 될까요~?

컨트롤러에 @validated로 설정되지 않은 곳에는 따로 그룹을 지정할 필요 없다고 생각해서 추가하지 않있습니다
이번에 해당 코드를 수정하면서 제가 찾아보기로는 @validated 가 아닌 경우는 그룹을 추가하지 않아도 검증 로직이 정상 작동 한다고 알고 있습니다. 그래서 추가할 이유가 없다고 판단되서요 !
혹시 추가해야하는 또 다른 이유가 있거나 제가 잘못알고 있다면 알려주시면 감사하겠습니다😆

Copy link
Contributor

@HoeSeong123 HoeSeong123 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

@zangsu zangsu merged commit f3cb161 into dev/be Sep 22, 2024
7 checks passed
@zangsu zangsu deleted the 667/fix-validated-setting branch September 22, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 bug 개발자가 의도하지 않은 상황
Projects
Status: Weekend Done
Development

Successfully merging this pull request may close these issues.

5 participants