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

[BE] refactor: Config 패키지 분리, 스타일 적용 #957

Merged
merged 4 commits into from
Nov 10, 2024

Conversation

donghoony
Copy link
Contributor

🚀 어떤 기능을 구현했나요 ?

  • Config 폴더 안에 너무 많은 일을 하고 있어서, 내부 패키지를 분리했습니다. WebConfig를 삭제하고, 각각의 자리에서 필요한 Config를 사용하도록 합니다. 글로벌 컨픽이라는 건 현재는 크게 없다고 보았어요.
  • Logger를 사용하는 곳이 좀 있어서 제거하고 @Slf4j를 적용했습니다.

📝 어떤 부분에 집중해서 리뷰해야 할까요?

  • 리뷰 그룹의 SessionResolver를 추가하는 WebConfigreviewgroup 패키지로 이동했어요. 이부분 어색하지 않은지만 확인해주세요.

📚 참고 자료, 할 말

  • 정돈된 코드가 좋아요, 백로그 하나씩 쳐내보겠습니다

Copy link

github-actions bot commented Nov 9, 2024

Test Results

154 tests  ±0   151 ✅ ±0   4s ⏱️ -1s
 59 suites ±0     3 💤 ±0 
 59 files   ±0     0 ❌ ±0 

Results for commit 2a4022d. ± Comparison against base commit cfdaeac.

This pull request removes 6 and adds 6 tests. Note that renamed tests count towards both.
reviewme.config.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_리뷰미_도메인_요청은_허락한다()
reviewme.config.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_외부_요청은_허락하지_않는다()
reviewme.config.LocalCorsConfigTest ‑ 로컬_프로파일에서는_외부_접근에_대해서도_허용한다()
reviewme.global.RequestLimitInterceptorTest ‑ POST_요청이_아니면_통과한다()
reviewme.global.RequestLimitInterceptorTest ‑ 특정_POST_요청이_처음이_아니며_최대_빈도보다_작을_경우_빈도를_1증가시킨다()
reviewme.global.RequestLimitInterceptorTest ‑ 특정_POST_요청이_처음이_아니며_최대_빈도보다_클_경우_예외를_발생시킨다()
reviewme.config.cors.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_리뷰미_도메인_요청은_허락한다()
reviewme.config.cors.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_외부_요청은_허락하지_않는다()
reviewme.config.cors.LocalCorsConfigTest ‑ 로컬_프로파일에서는_외부_접근에_대해서도_허용한다()
reviewme.config.requestlimit.RequestLimitInterceptorTest ‑ POST_요청이_아니면_통과한다()
reviewme.config.requestlimit.RequestLimitInterceptorTest ‑ 특정_POST_요청이_처음이_아니며_최대_빈도보다_작을_경우_빈도를_1증가시킨다()
reviewme.config.requestlimit.RequestLimitInterceptorTest ‑ 특정_POST_요청이_처음이_아니며_최대_빈도보다_클_경우_예외를_발생시킨다()
This pull request removes 3 skipped tests and adds 3 skipped tests. Note that renamed tests count towards both.
reviewme.config.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_리뷰미_도메인_요청은_허락한다()
reviewme.config.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_외부_요청은_허락하지_않는다()
reviewme.config.LocalCorsConfigTest ‑ 로컬_프로파일에서는_외부_접근에_대해서도_허용한다()
reviewme.config.cors.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_리뷰미_도메인_요청은_허락한다()
reviewme.config.cors.ExternalCorsConfigTest ‑ 로컬이_아닌_프로파일의_외부_요청은_허락하지_않는다()
reviewme.config.cors.LocalCorsConfigTest ‑ 로컬_프로파일에서는_외부_접근에_대해서도_허용한다()

♻️ This comment has been updated with latest results.

Copy link
Contributor

@skylar1220 skylar1220 left a comment

Choose a reason for hiding this comment

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

리뷰 그룹의 SessionResolver를 추가하는 WebConfig도 reviewgroup 패키지로 이동했어요. 이부분 어색하지 않은지만 확인해주세요.

ReviewGroupWebConfig은 reviewgroup 컨트롤러로 들어오는 과정에서만 사용되니 충분히 맞다고 느껴져요~

Copy link
Contributor

@Kimprodp Kimprodp left a comment

Choose a reason for hiding this comment

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

깔끔합니다 👍

Object value,
String message
) {
public record FieldErrorResponse(String field, Object value, String message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 왜 이렇게 변경했나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

한줄로 둬도 안 넘어서 그냥 펼쳤습니다 ㅎㅎ,,

Copy link
Contributor

@nayonsoso nayonsoso left a comment

Choose a reason for hiding this comment

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

Config 폴더 안에 너무 많은 일을 하고 있어서, 내부 패키지를 분리했습니다.

좋네요😄👍

—-

사실 처음에는 config 패키지가 '설정'만을 위한 패키지 같아서 RequestLimitInterceptor 와 TooManyRequestException 가 config 하위로 간다는게 어색하게 느껴지기도 했었는데요!

config 를 넓은 의미로 ‘어플리케이션의 설정’ 으로 본다면, 비니지스 로직과 설정을 분리하기 위한 것들로 해석될 수 있으니, 지금의 변화도 괜찮다 생각합니다😄👍 과도한 요청을 거부하는 것은 비지니스 로직은 아니니까요!

——-

리뷰 그룹의 SessionResolver를 추가하는 WebConfig도 reviewgroup 패키지로 이동했어요. 이부분 어색하지 않은지만 확인해주세요.

그래서 위와 비슷한 논리로, ‘리뷰 그룹의 SessionResolver를 추가하는 WebConfig’ 는 어플리케이션 자체에 대한 구성이므로 이전 위치가 더 적절했다고 생각해요.!

@donghoony
Copy link
Contributor Author

donghoony commented Nov 10, 2024

  • Interceptor/ArgumentsResolver 자체는 도메인 패키지 내부에 있는 게 자연스럽습니다. 하위 계층을 사용하기 때문입니다. (ReviewGroupSessionResolver의 경우 ReviewGroupService를 사용함)
  • 다만 이를 등록하는 Config는 도메인 하나에 귀속된다고 보기 어렵습니다.

따라서 WebConfig는 외부에 두고, 각각의 Interceptor/Resolver는 도메인 아래에 둡니다.

@donghoony donghoony merged commit 11a2b05 into develop Nov 10, 2024
4 checks passed
@donghoony donghoony deleted the be/refactor/polish branch November 10, 2024 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants