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

리뷰용 pr입니다. #96

Open
wants to merge 1 commit into
base: pr-review-base
Choose a base branch
from
Open

리뷰용 pr입니다. #96

wants to merge 1 commit into from

Conversation

hyeonic
Copy link

@hyeonic hyeonic commented Sep 1, 2024

#️⃣연관된 이슈

ex) #이슈번호, #이슈번호

📝작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

스크린샷 (선택)

💬리뷰 요구사항(선택)

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

ex) 메서드 XXX의 이름을 더 잘 짓고 싶은데 혹시 좋은 명칭이 있을까요?

@hyeonic hyeonic self-assigned this Sep 1, 2024
Comment on lines +6 to +13
@SpringBootApplication
public class KkeujeokBackendApplication {

public static void main(String[] args) {
SpringApplication.run(KkeujeokBackendApplication.class, args);
}

}
Copy link
Author

Choose a reason for hiding this comment

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

@SpringBootApplication 애노테이션의 역할은 무엇인가요??

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SpringBootApplication은 그 안에 중요한 어노테이션 3가지가 있습니다.

  1. @SpringBootConfiguration : 스프링 빈과 컨테이너 동작을 관리합니다.
  2. @EnableAutoConfiguration : 자동으로 필요한 설정을 구성하는데, 라이브러리와 연계해서 개발자가 개발만 집중하도록 주위 환경을 구성해 줍니다.
  3. @ ComponentScan : @component, @service, @controller, @repository 같은 어노테이션이 붙은 클래스를 찾아 자동으로 스프링 빈으로 등록합니다.

정리하자면 @SpringBootApplication을 사용하는 이유는 스프링 빈을 효과적으로 관리하고, 애플리케이션의 설정을 간소화하기 위해서입니다!
또 하나만 작성해야 하는 이유는 어떤 클래스를 기준으로 애플리케이션을 시작해야할지 혼란이 야기될 수 있기 때문입니다.
결론적으로 @SpringBootApplication을 사용하는 이유는 DI IOC를 효과적으로 구현하기 위함이라고 생각합니다!

Copy link
Author

Choose a reason for hiding this comment

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

좋습니다! 다만 조금 어색한 부분이 있어서 질문드려요.

결론적으로 @SpringBootApplication을 사용하는 이유는 DI IOC를 효과적으로 구현하기 위함이라고 생각합니다!

@SpringBootApplication 애노테이션은 이름에 따라 스프링 부트 사용 시 활용 될것으로 추측되는데요, 그렇다면 스프링 부트를 사용하는 이유가 IOC/DI와 어떠한 연관 관계를 가지고 있는건가요?? 스프링 부트를 사용해야만 IOC/DI를 활용할 수 있는건가요??

Comment on lines +32 to +42
@GetMapping("oauth2/callback/google")
public JsonNode googleCallback(@RequestParam(name = "code") String code) {
AuthService googleAuthService = authServiceFactory.getAuthService("google");
return googleAuthService.getIdToken(code);
}

@GetMapping("oauth2/callback/kakao")
public JsonNode kakaoCallback(@RequestParam(name = "code") String code) {
AuthService kakaoAuthService = authServiceFactory.getAuthService("kakao");
return kakaoAuthService.getIdToken(code);
}
Copy link
Author

Choose a reason for hiding this comment

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

만약 google, kakao 이외에 다른 로그인 방식이 생길 때는 어떻게 대응해야 할까요?? 이미 authServiceFactory를 통해 유동적으로 추가가 가능하다면 아래와 같이 하나의 컨트롤러로 충분히 대응가능 할 것 같아요!

    @GetMapping("oauth2/callback/{provider}")
    public JsonNode callback(@PathVariable(name = "provider") String provider,
                             @RequestParam(name = "code") String code) {
        AuthService authService = authServiceFactory.getAuthService(provider);
        return authService.getIdToken(code);
    }

Copy link
Author

Choose a reason for hiding this comment

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

추가로 JsonNode를 그대로 반환하는 이유가 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

구현에만 급급했던 것 같습니다..! 통합하는 컨트롤러로 바꿔보도록 하겠습니다!
JsonNode로 반환하는 부분도 DTO를 사용하는 방법으로 바꿔보겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

넵 좋습니다. DTO를 사용하는 이유에 대해 고민해보면 좋을 것 같아요!


Member member = getExistingMemberOrCreateNew(userInfo, provider);

validateSocialType(member, provider);
Copy link
Author

Choose a reason for hiding this comment

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

SocialType 정보를 가지고 있는 member에게 메시지를 전달하여 검증하는 방향은 어떠신가요?

getter를 사용하는 대신 객체에 메시지를 보내자

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋은 의견 감사합니다. 고민해 보겠습니다!

Comment on lines +15 to +21
@Autowired
public AuthServiceFactory(List<AuthService> authServiceList) {
authServiceMap = new HashMap<>();
for (AuthService authService : authServiceList) {
authServiceMap.put(authService.getProvider(), authService);
}
}
Copy link
Author

Choose a reason for hiding this comment

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

이건 취향이긴 한데 저는 변수명에 List와 같은 자료구조를 그대로 쓰는 것을 지양하고 있어요. 추후 List가 아닌 Set과 같이 타입이 변경되게 되면 변수명도 같이 수정해 주어야 하기 때문입니다.

복수 형태의 변수를 표현하기 위한 목적이라면 authServices와 같은 형태는 어떠신가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

항상 추천하는 이름으로 쓰다보니 자연스럽게 List로 사용했던 것 같습니다.
후에 바꿀 가능성을 생각해서 다른 이름 고민해 보도록 하겠습니다. 감사합니다!

Comment on lines +31 to +60
@PostMapping("/")
public RspTemplate<BlockInfoResDto> save(@CurrentUserEmail String email,
@RequestBody BlockSaveReqDto blockSaveReqDto) {
return new RspTemplate<>(HttpStatus.CREATED, "블럭 생성", blockService.save(email, blockSaveReqDto));
}

@PatchMapping("/{blockId}")
public RspTemplate<BlockInfoResDto> update(@CurrentUserEmail String email,
@PathVariable(name = "blockId") Long blockId,
@RequestBody BlockUpdateReqDto blockUpdateReqDto) {
return new RspTemplate<>(HttpStatus.OK, "블록 수정", blockService.update(email, blockId, blockUpdateReqDto));
}

@PatchMapping("/{blockId}/progress")
public RspTemplate<BlockInfoResDto> progressUpdate(@CurrentUserEmail String email,
@PathVariable(name = "blockId") Long blockId,
@RequestParam(name = "progress") String progress) {
return new RspTemplate<>(HttpStatus.OK, "블록 상태 수정", blockService.progressUpdate(email, blockId, progress));
}

@GetMapping("")
public RspTemplate<BlockListResDto> findForBlockByProgress(@CurrentUserEmail String email,
@RequestParam(name = "dashboardId") Long dashboardId,
@RequestParam(name = "progress") String progress,
@RequestParam(name = "page", defaultValue = "0") int page,
@RequestParam(name = "size", defaultValue = "10") int size) {
return new RspTemplate<>(HttpStatus.OK,
"블록 상태별 전체 조회",
blockService.findForBlockByProgress(email, dashboardId, progress, PageRequest.of(page, size)));
}
Copy link
Author

Choose a reason for hiding this comment

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

("")("/")는 생략이 가능할 것 같아요. 혹은 통일하는 방향은 어떠신가요?

Copy link
Member

Choose a reason for hiding this comment

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

통일하는 방향으로 다음 회의때 이야기 해보겠습니다!

Comment on lines +3 to +17
public enum Category {

HEALTH_AND_FITNESS("건강 및 운동"),
MENTAL_WELLNESS("정신 및 마음관리"),
PRODUCTIVITY_AND_TIME_MANAGEMENT("생산성 및 시간 관리"),
FINANCE_AND_ASSET_MANAGEMENT("재정 및 자산 관리"),
SELF_DEVELOPMENT("자기 개발"),
LIFE_ORGANIZATION_AND_MANAGEMENT("생활 정리 및 관리"),
SOCIAL_CONNECTIONS("사회적 연결"),
CREATIVITY_AND_ARTS("창의력 및 예술 활동"),
OTHERS("기타");

Category(String description) {
}
}
Copy link
Author

Choose a reason for hiding this comment

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

Category의 개수는 제한적인가요?? 만약 서버가 운영되고 있을 때 Category가 추가되어야 하는 상황이 발생할 수 있을까요?

Copy link
Member

Choose a reason for hiding this comment

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

넵, 현재 Category는 enum으로 구현되어 있기 때문에 개수는 제한적입니다. 미리 정의된 상수 집합을 사용하므로, 동적으로 추가가 불가능하고 만약 Category가 추가되어야 하는 상황이 발생했을때는 코드를 수정 후 재배포를 해야 합니다.

Comment on lines +109 to +111
long completedBlocks = blocks.stream()
.filter(b -> b.getProgress().equals(Progress.COMPLETED))
.count();
Copy link
Author

Choose a reason for hiding this comment

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

enum을 통해 equals 비교를 할 때와 == 비교를 할 때 차이가 있을까요?? enum의 특성에 대해 학습해보면 좋을 것 같습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

기본적으로 enum은 상수의 집합입니다. 각각의 상수는 하나의 인스턴스라서 하나의 객체 즉 enum은 싱글톤과 유사한 방식으로 관리됩니다. 때문에 주소값이 같으므로 ==으로 비교가 가능합니다.
enum의 equlas 매서드를 보니까 안에 ==으로 구현되어 있는 것을 보았습니다.
때문에 결론은 enum을 통해 equals 비교를 할 때와 == 비교를 할 때 차이가 없습니다!

Comment on lines +91 to +100
@Transactional
public void delete(String email, Long dashboardId) {
Member member = memberRepository.findByEmail(email).orElseThrow(MemberNotFoundException::new);
PersonalDashboard dashboard = personalDashboardRepository.findById(dashboardId)
.orElseThrow(DashboardNotFoundException::new);

verifyMemberIsAuthor(dashboard, member);

dashboard.statusUpdate();
}
Copy link
Author

Choose a reason for hiding this comment

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

논리적인 삭제와 물리적인 삭제는 어떠한 차이와 장단점을 가지고 있을까요?

Copy link
Member

@giwoong01 giwoong01 Sep 5, 2024

Choose a reason for hiding this comment

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

논리 삭제는 데이터베이스에 데이터를 유지할 수 있습니다.
저희가 논리 삭제를 진행한 이유는 휴지통이라는 기능이 있기 때문입니다.
휴지통에서는 삭제되었던 블록, 대시보드 등을 복구할 수 있습니다.
그렇기 때문에 데이터를 유지하려 했습니다.

단점으로는 데이터베이스의 성능이 저하되거나 크기가 늘어나겠네요!


물리적으로 삭제하게되면 데이터의 유지가 어렵습니다. 저희의 서비스 특성상 휴지통에서 복구를 해야하는데, 그게 불가능하게되죠!
물론 데이터베이스의 성능이 좋아지고, 저장 공간을 절약할 수 있어 좋겠네요.

그래서 결과적으로 저희는 데이터를 복구하기 위해서 논리 삭제를 선택했습니다!!

Comment on lines +11 to +34
@Configuration
@RequiredArgsConstructor
public class FilterWebConfig {

private final TokenProvider tokenProvider;

@Bean
public FilterRegistrationBean<LogFilter> logFilter() {
FilterRegistrationBean<LogFilter> filterRegistrationBean = new FilterRegistrationBean<>();
filterRegistrationBean.setFilter(new LogFilter()); // 여기서 만든 필터 클래스 등록
filterRegistrationBean.setOrder(1);
filterRegistrationBean.addUrlPatterns("/*");
return filterRegistrationBean;
}

@Bean
public FilterRegistrationBean<LoginCheckFilter> loginCheckFilter() {
FilterRegistrationBean<LoginCheckFilter> filterRegistrationBean = new FilterRegistrationBean<>();
filterRegistrationBean.setFilter(new LoginCheckFilter(tokenProvider)); // JWT 토큰 유효성 검사를 위한 필터 클래스 등록
filterRegistrationBean.setOrder(2); // 1번인 로그필터 다음으로 수행
filterRegistrationBean.addUrlPatterns("/*");
return filterRegistrationBean;
}
}
Copy link
Author

Choose a reason for hiding this comment

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

필터에서 로그인 체크를 진행하고 있네요! 필터는 사용자에게 요청이 들어온 후 어느 시점에 동작하게 될까요? interceptor, argumentResolver등과 같이 흐름을 쭉 정리해보면 좋을 것 같습니다!

Copy link
Member

Choose a reason for hiding this comment

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

필터는 스프링 컨텍스트에 가기 전 웹 컨텍스트에서 처리됩니다.
그 후 스프링 컨텍스트 내부에서 서블릿 -> 인터셉터 -> 컨트롤러 순으로 동작이 되죠.

필터는 웹 컨텍스트에서 동작하기 때문에 에러가 발생하면 스프링 부트에서 처리하지 못합니다!
그래서 필터에서 에러가 발생하면 직접 응답을 만들어서 출력해주어야 합니다!

Copy link
Author

Choose a reason for hiding this comment

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

웹 컨텍스트, 스프링 컨텍스트는 무엇인가요?? 그렇다면 서블릿이나 필터는 스프링에 종속된 개념일까요??

Comment on lines +6 to +9
@Component
@EnableScheduling
public class SchedulerConfig {
}
Copy link
Author

Choose a reason for hiding this comment

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

@Component보단 설정 관련이라 @Configuration이 적합해보이네요. 두 애노테이션은 어떤 차이가 있을까요?

Copy link
Collaborator

@inhooo00 inhooo00 Sep 6, 2024

Choose a reason for hiding this comment

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

지금까지는 애플리케이션이 실행되기 전에 진행하는 로직으로만 인지하고 있었기에 추가적으로 공부해 보았습니다!
흘러가는 로직을 설명해 보겠습니다.

@component

  • @Component 가 정의된 클래스를 찾을때는 @ComponentScan 어노테이션을 이용하여 찾게됨. : @SpringBootApplication 안에 구현되어 있는 그거.
  • @ComponentScan 로 감지된 클래스는 Bean으로 등록되어 사용됨.

@configuration

  • @Configuration 이 선언된 클래스는 하나 이상의 @Bean 메소드를 선언하고 해당 @Bean 메소드는 스프링 컨테이너를 통해 처리됨.
  • @Configuration내부에 @Component를 가지고 있기에 @Configuration 이 선언된 클래스도 Bean으로 등록.
  • 또한, @Bean 메소드는 @Configuration*내부에서 선언해야 Singleton Scope 를 가질 수 있다!
    • 바로 @Configuration 의 proxyBeanMethods 설정 때문.
    • 이 설정 때문에 @Configuration@Bean은 세트다. 싱글톤 보장을 해주기 때문에~~

정리하자면 @Component는 클래스 레벨에서 사용할 수 있고, 우리가 직접 구현한 클래스를 빈으로 등록할 때 사용합니다. (자동)
@Configuration은 클래스 레벨에서 사용하는 것은 같지만 @Bean과 연계하여 메서드 내부에서 생성한 객체를 Bean으로 등록할 때 사용합니다. (수동)

결론

  • 물론 기능적으로는 지금 @Bean으로 선언한 메서드가 없기 때문에 상관 없지만 스프링의 설정 역할을 수행하는 클래스에는 @Configuration`가 맞는 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

좋습니다~ 자주 활용되고 있는 @Component, @Service, @Repository도 각각의 주석이 각각 어떠한 의미를 하게 되는지도 함께 알아보면 좋을 것 같습니다!

Copy link
Collaborator

@inhooo00 inhooo00 left a comment

Choose a reason for hiding this comment

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

리팩토링 들어가겠습니다!!

Comment on lines +6 to +13
@SpringBootApplication
public class KkeujeokBackendApplication {

public static void main(String[] args) {
SpringApplication.run(KkeujeokBackendApplication.class, args);
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SpringBootApplication은 그 안에 중요한 어노테이션 3가지가 있습니다.

  1. @SpringBootConfiguration : 스프링 빈과 컨테이너 동작을 관리합니다.
  2. @EnableAutoConfiguration : 자동으로 필요한 설정을 구성하는데, 라이브러리와 연계해서 개발자가 개발만 집중하도록 주위 환경을 구성해 줍니다.
  3. @ ComponentScan : @component, @service, @controller, @repository 같은 어노테이션이 붙은 클래스를 찾아 자동으로 스프링 빈으로 등록합니다.

정리하자면 @SpringBootApplication을 사용하는 이유는 스프링 빈을 효과적으로 관리하고, 애플리케이션의 설정을 간소화하기 위해서입니다!
또 하나만 작성해야 하는 이유는 어떤 클래스를 기준으로 애플리케이션을 시작해야할지 혼란이 야기될 수 있기 때문입니다.
결론적으로 @SpringBootApplication을 사용하는 이유는 DI IOC를 효과적으로 구현하기 위함이라고 생각합니다!

Comment on lines +32 to +42
@GetMapping("oauth2/callback/google")
public JsonNode googleCallback(@RequestParam(name = "code") String code) {
AuthService googleAuthService = authServiceFactory.getAuthService("google");
return googleAuthService.getIdToken(code);
}

@GetMapping("oauth2/callback/kakao")
public JsonNode kakaoCallback(@RequestParam(name = "code") String code) {
AuthService kakaoAuthService = authServiceFactory.getAuthService("kakao");
return kakaoAuthService.getIdToken(code);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

구현에만 급급했던 것 같습니다..! 통합하는 컨트롤러로 바꿔보도록 하겠습니다!
JsonNode로 반환하는 부분도 DTO를 사용하는 방법으로 바꿔보겠습니다.


Member member = getExistingMemberOrCreateNew(userInfo, provider);

validateSocialType(member, provider);
Copy link
Collaborator

Choose a reason for hiding this comment

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

좋은 의견 감사합니다. 고민해 보겠습니다!

Comment on lines +15 to +21
@Autowired
public AuthServiceFactory(List<AuthService> authServiceList) {
authServiceMap = new HashMap<>();
for (AuthService authService : authServiceList) {
authServiceMap.put(authService.getProvider(), authService);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

항상 추천하는 이름으로 쓰다보니 자연스럽게 List로 사용했던 것 같습니다.
후에 바꿀 가능성을 생각해서 다른 이름 고민해 보도록 하겠습니다. 감사합니다!

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.

4 participants