Skip to content

Conversation

@Jaeyeop-Jung
Copy link
Contributor

관련 이슈 번호

BE-72 / OauthService 구현체 및 테스트 코드 작성

설명

  • ConfigurationProperties를 이용해 Google과 Kakao의 Oauth 관련 환경변수 불러오도록 설정
  • OauthConstants를 추가해 Oauth 요청시 QuerParameter에 들어가는 Key, Value 정의
  • Generic으로 구현한 CustomObjectMapper으로 check Exception(JsonProcessingException)을 Uncheck Exception으로 전환하도록 함
  • Google, Kakao Oauth 요청시 응답 받을 DTO 추가
  • RestTemplate에서 발생가능한 RestClientException Handler에 추가

변경사항

위 사항

질문사항

Copy link
Member

@kdomo kdomo left a comment

Choose a reason for hiding this comment

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

테스트 코드는 추후에 작성되어야 되는거죠??

try {
return objectMapper.readValue(str, clazz);
} catch (JsonProcessingException e) {
throw new IllegalArgumentException("잘못된 Json값이나 Class Type을 입력했습니다.");
Copy link
Member

Choose a reason for hiding this comment

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

IllegalArgumentException 대신에 Custom Exception을 쓰는게 어떨까요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Custom exception을 넣을까 고민해봤는데 결국 CustomException을 넣는다한들 달라지는게 없어보여서 기본 예외로 처리했습니다

결국 저 코드에서 JsonProcessException이 터지면 그것은 애초에 개발자가 저 메소드에 넘긴 인자가 잘못된 경우 밖에 없을거라 판단했고(사용자의 오류가 아닌 컴파일 에러지만 잡히지 않는 개발자의 오류), Custom 처리를 한다고해도 저 메소드의 호출부가 문제일테니 거기에 예외처리를 둬야될 것 같아용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

정리하자면

Custom Exception을 쓰던, 기본 예외를 쓰던 결국 호출부를 잡지 않으면 상관이 없다

더 완벽하게 이 예외를 처리하고 싶으면 해당 메소드를 호출하는 호출부를 try catch로 묶어서 거기서는 custom exception을 처리해도 된다 입니당

Copy link
Member

Choose a reason for hiding this comment

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

exceptionHandler를 정의하고 customException 을 정의한 이유가
프론트엔드에서 공통된 에러메세지의 형태로 주기 위해서 였는데,
IllegalArgumentException를 발생시킨다면 Handler에서 잡히지 않고
프론트엔드가 분기처리하기 애매해지지 않을까요?

라이언 생각은 개발단계에서만 발생하는 오류이기 때문에 IllegalArgumentException로 던져도 상관없다 라는뜻인가요?

Comment on lines 1 to 40
package com.recordit.server.dummy;

import java.util.Objects;

public class Foo {
private String field1;
private String field2;

public Foo(String field1, String field2) {
this.field1 = field1;
this.field2 = field2;
}

public Foo() {
}

public String getField1() {
return field1;
}

public String getField2() {
return field2;
}

@Override
public boolean equals(Object o) {
if (this == o)
return true;
if (o == null || getClass() != o.getClass())
return false;
Foo foo = (Foo)o;
return Objects.equals(getField1(), foo.getField1()) && Objects.equals(getField2(),
foo.getField2());
}

@Override
public int hashCode() {
return Objects.hash(getField1(), getField2());
}
}
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
Contributor Author

Choose a reason for hiding this comment

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

이거는 CustomObjectMapper 테스트용으로 쓰려고 의도적으로 넣어논 클래스입니다! 기존의 DTO 코드를 쓰기에는 나중에 변경이 될 수 있을 것 같아서 테스트 전용 클래스를 하나 만든거에용

Copy link
Member

Choose a reason for hiding this comment

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

해당 클래스를 꼭 사용해야 될까요? Foo 라는 네이밍 자체가 애매하네요..
테스트케이스 내부 클래스로 선언하는게 좋아보입니다!

@Jaeyeop-Jung
Copy link
Contributor Author

테스트 코드는 추후에 작성되어야 되는거죠??

테스트 코드는 제가 작성하려고 했는데 결국 UriComponentBuilder나 RestTemplate을 테스트하는 꼴이 되어버려서 제가 일부로 안넣었습니당

Comment on lines 1 to 40
package com.recordit.server.dummy;

import java.util.Objects;

public class Foo {
private String field1;
private String field2;

public Foo(String field1, String field2) {
this.field1 = field1;
this.field2 = field2;
}

public Foo() {
}

public String getField1() {
return field1;
}

public String getField2() {
return field2;
}

@Override
public boolean equals(Object o) {
if (this == o)
return true;
if (o == null || getClass() != o.getClass())
return false;
Foo foo = (Foo)o;
return Objects.equals(getField1(), foo.getField1()) && Objects.equals(getField2(),
foo.getField2());
}

@Override
public int hashCode() {
return Objects.hash(getField1(), getField2());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

해당 클래스를 꼭 사용해야 될까요? Foo 라는 네이밍 자체가 애매하네요..
테스트케이스 내부 클래스로 선언하는게 좋아보입니다!

try {
return objectMapper.readValue(str, clazz);
} catch (JsonProcessingException e) {
throw new IllegalArgumentException("잘못된 Json값이나 Class Type을 입력했습니다.");
Copy link
Member

Choose a reason for hiding this comment

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

exceptionHandler를 정의하고 customException 을 정의한 이유가
프론트엔드에서 공통된 에러메세지의 형태로 주기 위해서 였는데,
IllegalArgumentException를 발생시킨다면 Handler에서 잡히지 않고
프론트엔드가 분기처리하기 애매해지지 않을까요?

라이언 생각은 개발단계에서만 발생하는 오류이기 때문에 IllegalArgumentException로 던져도 상관없다 라는뜻인가요?

@Jaeyeop-Jung Jaeyeop-Jung requested a review from kdomo December 26, 2022 05:15
@kdomo kdomo merged commit 86da281 into develop Dec 26, 2022
@kdomo kdomo deleted the feature/BE-72 branch December 26, 2022 05:53
@kdomo kdomo mentioned this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ Feature 기능 개발 🛠 Settings 개발 환경 설정

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants