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

코드 리뷰~ #4

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

코드 리뷰~ #4

wants to merge 5 commits into from

Conversation

parksooo
Copy link
Contributor

@parksooo parksooo commented Sep 2, 2024

1주차 9/9 - 9/14

  • Member, Location
  • suwhpark, jnam

2주차 9/19 - 9/25

  • Group, Groupmember
  • jonhan

3주차 9/30 - 10/7

  • OAuthToken, Api, Search
  • suwhpark, hjeong

4주차 10/7 - 10/13

  • Join, Exception
  • suwhpark

5주차 10/14 -

  • Security, Jwt, Auth, Logout
  • suwhpark

@parksooo parksooo linked an issue Sep 2, 2024 that may be closed by this pull request
@OneToMany(mappedBy = "member", cascade = CascadeType.ALL)
private List<GroupMember> groupMembers = new ArrayList<>();

public Member(final CadetPrivacy cadetPrivacy, final Hane hane) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CardetPrivacy를 만들어서 이 객체로 멤버를 만드는 이유가 뭔가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cadetPrivacy는 42api에 요청하여 받아온 카뎃 정보입니다
42 정보 기반으로 저희 서비스의 member를 만들게 되는거죠

@RestController
@RequestMapping("/v3/member")
@RequiredArgsConstructor
public class MemberController implements MemberApiDocs {
Copy link
Collaborator

Choose a reason for hiding this comment

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

인터페이스를 implements로 상속받아서 쓰는 것 까진 이해했는데 apiDocs를 상속 받는 이유가 뭔가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

인터페이스는 하나의 명세서라고 생각하면 편해요.
예를 들어, 우리가 controller에 method를 만들려고하는데, 그거에 대한 설명이나 필요한 파라미터 등의 정보를 인터페이스에 구현해 놓고 상속 받아 사용하는 겁니다.
apiDocs는 swagger 설정을 적어놓는 역할이고, 저희는 스웨거 설정 어노테이션이 너무 길어서 클린코드 지향하려 상속받았습니다

private final LocationService locationService;

/**
* 수동자리 업데이트
Copy link
Collaborator

Choose a reason for hiding this comment

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

수동자리 업데이트는 유저가 수동으로 자리 설정 할 때 호출되는 api인가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 맞습니다~

* @return ResponseEntity(ResponseMemberDTO)
*/
@GetMapping("/one")
public ResponseEntity<ResponseMemberDTO> findOneByIntraId(@RequestParam("intraId") final Integer intraId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@RequestParam("intraId")
intraid가 쿼리로 들어오는 것 같은데 "v3/member/one/{id}" 이렇게 요청이 들어온다는 건가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

음 수하님이 말씀하신 방식은

@GetMapping("/one/{id}")
    public String getUserById(@PathVariable Long id) {
        //서비스
    }

이 방식입니다
저희가 사용하는 것은 "/v3/member/one/intraid?intraid=suhwpark" 이렇게 쿼리 형식이 갈꺼에용

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 제가 쿼리랑 파람이 같은건줄 알았습니다! 예시까지..! 덕분에 이해가 완벽히 됬습니당 감사합니당!!

Choose a reason for hiding this comment

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

굿굿! 이게 스프링에서는 파람, 다른 프레임워크에서는 쿼리 이렇게일거에요 아마도~

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 추가적인 설명 감사합니당!!

this.image = member.getImage();
this.comment = member.getComment();
this.inCluster = member.isInCluster();
this.agree = member.isAgree();
Copy link
Collaborator

Choose a reason for hiding this comment

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

agree 변수는 어떤 동의를 말하는 건가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where42는 개인 위치 정보 제공하는 서비스여서 서비스 이용 동의가 필요합니다
만약에 서비스 이용 동의를 하지 않는다면, 그냥 비동의 맴버로 만들어놓고 사용합니다
비동의 맴버를 사용하는 곳은 search에서 where42 동의하지 않은 카뎃을 검색하고 mac 자리를 검색할 때 사용해요!
search 한 결과를 json으로 만들어서 보내는 것보다, 가입할 수도 있는 카뎃이니 agree 컬럼으로 관리합니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 이해했습니당 감사합니다!

Copy link

@namzisun namzisun Sep 19, 2024

Choose a reason for hiding this comment

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

agree : hane API 로 클러스터 출입 여부 조회하는 기준

  • false : hane 조회 X (출퇴근 여부 알 수 없음) / 42 api 조회 O (클러스터 맥 정보 알 수 있음)
  • true : hane 조회 O (출퇴근 여부 알 수 있음) / 42 api 조회 O (클러스터 맥 정보 알 수 있음)

즉, 서비스 내에서 hane API 에 대해서 agree=true인 경우만 조회함!

이렇게 활용합니다!


public MemberException(final MemberErrorCode memberErrorCode) {
super(memberErrorCode);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

아래 static으로 된 메서드를 호출하면 될 것같은데 MemberException(MemverErrorCode) 생성자를 만들어주는 이유가 궁금합니다. 따로 사용되는 곳을 못 찾겠습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

static 매서드가 MemberException을 상속 받아서 만든 클래스여서 필요합니다
구조를 그려보자면
CustomException
⬇️
MemberException
⬇️
NoMemerException (맴버 exception을 세분화 한다고 생각하면됌)
이렇게 되어있습니다
저희 의도는 맴버 exception에서 발생할 수 있는 모든 에러를 상속받아 구현하려고 했습니다~

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 version 예외를 구현 할 때 이 폼을 참고했다면 더 예쁜 코드가 됐을것같네요 감사합니다 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 version 객체도 이렇게 돼있군요 ..

private final GroupService groupService;
private final GroupMemberService groupMemberService;

@PostMapping("")
Copy link
Collaborator

Choose a reason for hiding this comment

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

어떻게 구글링 해봐야할지도 잘 모르겠어서 질문 남깁니다.
같은 url에 대해 어떤 종류의(post, get 등) 요청인지로 구분하는것이 지향해야할 방법인가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

음 http의 method로 구별하기 때문에 굳이, get post 등의 url을 구분하는 것은 자원 낭비 라고 저희 팀은 생각했어요
그리고 restApi로 "post" v3/api/group 이렇게 요청하면, 아 그룹에서 변경점이 일어나는군 이라고 명시적으로 알 수 있는 장점도 있어서 구분하지 않았습니다.

Copy link

@namzisun namzisun Sep 25, 2024

Choose a reason for hiding this comment

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

REST API URI Naming Conventions

요 문서의 2.4번을 봐주세요!
저희가 모든 컨벤션을 따른것은 아닙니다만,

  1. request를 구성하는 요소는 url 하나가 아니라 HTTP request 전체이기 때문에 같은 자원에 대한 post/get/delete는 url이 아닌 method로 구별하는 것이 맞다고 판단했습니다.
  2. 또한 url가 너무 세분화되면 사용할때도, 만들때도 헷갈리더라구요! 때문에 최대한 간단하고 직관적으로 구성하려고 했습니다.

상기 이유로 동일 자원에 대한 url은 method로 구분했습니다. 다만 컨벤션 문서를 지금 다시 정독해보니 개발 후반부로 갈수록 컨벤션을 엄격하게 고려하지 않은 url도 있네요. 컨벤션은 컨벤션일 뿐이니 3기 기준에 맞지 않다고 판단되는 부분은 프론트와 협의 후 수정하시면 될 것 같습니다~!


Optional<Group> findById(Long groupId);

@Query("SELECT gm.group FROM GroupMember gm " +
Copy link
Collaborator

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.

해당 인트라 아이디의 Member가 만든 모든 그룹을 찾아서 반환하는 쿼리 문입니다.
따로 쿼리를 작성하지 않으면, 직접 코드로 group을 걸러내야하기에 한번에 자신이 소유한 group을 조회하려고 직접 JPQL을 작성했습니다~

summary = "2.2 그룹 목록 및 친구 API 조회",
description = "멤버가 만든 그룹 및 그룹 내의 친구 목록을 조회합니다 (메인 화면 용)",
responses = {
@ApiResponse(responseCode = "200", description = "조회 성공", content = @Content(schema = @Schema(implementation = ResponseGroupMemberListDTO.class))),
Copy link
Collaborator

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.

따로 공부하기 보다는, 찾아서 사용했던거 같아요
위에 같은 경우는 spring에서 지원하는 어노테이션이 아닌 swagger에서 제공하는 어노테이션이에요
swagger 페이지 들어가면, 상세 정보 적혀있는게 위와 같이 설정하는 거라고 보시면 편해요

* @param authUser
*/
public final void updateGroupValidate(final UpdateGroupDTO dto, final AuthUser authUser) {
if (dto.getGroupId().equals(authUser.getDefaultGroupId()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

dto의 groupId와 유저의 defaultGroupId가 같으면 왜 예외를 던지는지 모르겠습니다. Auth.defaultGroupId가 어떤 정보를 의미하는 건가요?

Copy link

@namzisun namzisun Sep 26, 2024

Choose a reason for hiding this comment

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

default group은 친구 목록입니다!
때문에 그룹 이름을 바꾸는 등의 커스텀한 작업은 불가능합니다
default group : create member시에 자동 생성되는 기본 그룹. 서비스에서 친구 목록의 역할을 함

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 이해됐습니다! 감사합니다!


@JsonProperty(value = "isOwner")
@Column(name = "is_owner")
private Boolean isOwner;
Copy link
Collaborator

Choose a reason for hiding this comment

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

isOwner의 의미가 제가 이해한것이 맞는지 모르겠습니다
groupMember에 멤버가 sooha면서 IsOwner가 true인 groupMember의 group들은 sooha계정에서 만들어진 그룹이라는 의미일까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

네 맞아요 해당 그룹의 주인을 뜻하는 속성이에요

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 +181 to +185
public void duplicateGroupMember(final Long groupId, final List<Member> members){
final long count = groupMemberRepository.countByGroup_GroupIdAndMemberIn(groupId, members);
if (count > 0)
throw new GroupMemberException.DuplicatedGroupMemberException();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

새로운(a,b,c친구들) 친구들을 추가하려는 그룹에 새로운 친구들중(a,b,c중 한명이상) 한명이라도 있으면 예외를 던지는 건가요..?

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
Collaborator

Choose a reason for hiding this comment

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

아하 감사합니다!

import kr.where.backend.api.exception.JsonException;

public class JsonMapper {
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 ObjectMapper의 개체가 대문자와 스네이크케이스로 작성된 이유가 뭔가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

자바 컨벤션은 상수로 정의할 경우 스네이크 케이스를 사용합니다!

@Getter
@AllArgsConstructor
public class UpdateGroupDTO {
@NotNull
Copy link
Collaborator

Choose a reason for hiding this comment

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

코드들을 다시 보다가 생각한건데 NotNull이나 NotBlank같은 어노테이션들은 설계 단계에서 또는 개발 초기에 생각나면 명시하나요??
쉽게 놓칠 수 있는 부분이라고 생각돼서 여쭤봅니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저희는 개발 초기 단계에서는 확인하지 못했고, exception 처리하는 과정에서 dto의 유효성을 검사하고 싶어
어노테아션을 붙이게 되었습니다

UriBuilder.hane(Uri.HANE_INFO_LIST.getValue())), HaneResponseDto[].class);
} catch (final RequestException exception) {
log.warn("[hane] : {}", exception.toString());
return new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

빈 배열을 리턴하는 이유는 단순히 반환값을 맞추려고 일까요??
상위메서드에서 호출하면 빈 배열이니 밖에서는 에러가 안뜨는게 목적이였을까요??

아니면 자바 문법상 반환값이 있는 메서드들은 catch에서도 리턴을 꼭 해야하나요???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이것은 의도된것입니다.
자바 문법상 catch 했을 경우, throw를 하거나, return을 하거나 자유롭게 할 수 있습니다.
하지만 이 method는 외부 API 요청을 하네에게 하는 것이기에 우리 서비스 단의 에러라고 판단하지 않습니다.
그래서 하네측에서 에러가 날 경우, 빈 문자열을 반환하는 것입니다 그래서 log도 warn으로 설정합니다.

@Getter
@JsonIgnoreProperties(ignoreUnknown = true)
public class Versions {
private String small;
Copy link
Collaborator

Choose a reason for hiding this comment

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

small 필드는 Image의 url 일까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{
  "id": 2,
  "email": "[[email protected]](mailto:[email protected])",
  "login": "andre",
  "first_name": "André",
  "last_name": "Aubin",
  "usual_full_name": "Juliette Aubin",
  "usual_first_name": "Juliette",
  "url": "https://api.intra.42.fr/v2/users/andre",
  "phone": null,
  "displayname": "André Aubin",
  "kind": "admin",
  "image": {
    "link": "https://cdn.intra.42.fr/users/1234567890/andre.jpg",
    "versions": {
      "large": "https://cdn.intra.42.fr/users/1234567890/large_andre.jpg",
      "medium": "https://cdn.intra.42.fr/users/1234567890/medium_andre.jpg",
      "small": "https://cdn.intra.42.fr/users/1234567890/small_andre.jpg",
      "micro": "https://cdn.intra.42.fr/users/1234567890/micro_andre.jpgg"
    }
  },
  "staff?": false,
  "correction_point": 4,
  "pool_month": "july",
  "pool_year": "2016",
  "location": null,
  "wallet": 0,
  "anonymize_date": "2021-02-20T00:00:00.000+03:00",
  "data_erasure_date": null,
  "alumni?": false,
  "active?": true,
  "groups": [],
  "cursus_users": [
    {
      "id": 2,
      "begin_at": "2017-05-14T21:37:50.172Z",
      "end_at": null,
      "grade": null,
      "level": 0.0,
      "skills": [],
      "cursus_id": 1,
      "has_coalition": true,
      "user": {
        "id": 2,
        "login": "andre",
        "url": "https://api.intra.42.fr/v2/users/andre"
      },
      "cursus": {
        "id": 1,
        "created_at": "2017-11-22T13:41:00.750Z",
        "name": "Piscine C",
        "slug": "piscine-c"
      }
    }
  ],
  "projects_users": [],
  "languages_users": [
    {
      "id": 2,
      "language_id": 3,
      "user_id": 2,
      "position": 1,
      "created_at": "2017-11-22T13:41:03.638Z"
    }
  ],
  "achievements": [],
  "titles": [],
  "titles_users": [],
  "partnerships": [],
  "patroned": [
    {
      "id": 4,
      "user_id": 2,
      "godfather_id": 15,
      "ongoing": true,
      "created_at": "2017-11-22T13:42:11.565Z",
      "updated_at": "2017-11-22T13:42:11.572Z"
    }
  ],
  "patroning": [],
  "expertises_users": [
    {
      "id": 2,
      "expertise_id": 3,
      "interested": false,
      "value": 2,
      "contact_me": false,
      "created_at": "2017-11-22T13:41:22.504Z",
      "user_id": 2
    }
  ],
  "roles": [],
  "campus": [
    {
      "id": 1,
      "name": "Cluj",
      "time_zone": "Europe/Bucharest",
      "language": {
        "id": 3,
        "name": "Romanian",
        "identifier": "ro",
        "created_at": "2017-11-22T13:40:59.468Z",
        "updated_at": "2017-11-22T13:41:26.139Z"
      },
      "users_count": 28,
      "vogsphere_id": 1
    }
  ],
  "campus_users": [
    {
      "id": 2,
      "user_id": 2,
      "campus_id": 1,
      "is_primary": true
    }
  ]
}

42api에 v2/me에 요청하면 위와 같은Json형식으로 정보를 제공해줍니다.
여기서 우리는 필요한 정보만 mapping 해야 효율이 좋아집니다.
이미지가 필요하기에 이미지의 버전에 대한 객체를 만들고 그 필드인 samll을 설정하면, 작은 이미지만 받아올수 있게 됩니다~

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 +34 to +44
/**
* code로 OAuth token 받아와서 생성 & 업데이트
* @param name : 생성할 token 이름
* @param code : log에 찍힌 code
*/
@PostMapping("")
public ResponseEntity createToken(@RequestParam("name") String name, @RequestParam("code") String code) {
final OAuthTokenDto oAuthToken = tokenApiService.getOAuthToken(code);
oauthTokenService.createToken(name, oAuthToken);
return new ResponseEntity(HttpStatus.CREATED);
}
Copy link
Collaborator

@leeesooha leeesooha Oct 3, 2024

Choose a reason for hiding this comment

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

@hani-j background API에 대해 search, image 갱신, admin 요청마다 토큰을 따로 발급 받는것으로 설명들었습니다. 제 짧은 식견으론 background API에 대해 한개에 토큰만 발급받아 사용해도 될것 같은데 왜 3개로 토큰을 나누어 요청을 할까요..?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@leeesooha admin은 권한 때문에 분리 돼있는게 옳은 방향이라고 생각합니다. 근데 이미지랑 서치는 ...

Copy link

Choose a reason for hiding this comment

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

42 api 가 1초당 3번 제한이 있어서 동시에 사용할 가능성이 있는 경우를 분리했어요
스케줄링 따로, 서치 따로, image 는 지금은 사용할 일이 없는데 새로운 기수가 들어오면 한 번에 업데이트 해 놓는 용도로 존재합니다! admin 이랑 search 만 있어도 되긴 해요

Copy link
Collaborator

Choose a reason for hiding this comment

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

토큰하나로 어플리케이션 3개를 다 사용할 수 있을 거같은데 토큰도 3개로 분류한 이유가 궁금합니다!
혹시 어플리케이션당 1초3번제한이 아닌 토큰당1초3번제한일까요?

Copy link

Choose a reason for hiding this comment

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

아? 맞네요ㅋㅋㅋ 그땐 왜 토큰 당이라 생각했을까요..
어플리케이션당 맞습니다! 토큰 하나로도 다 가능합니다!
만약에라도 1초 2회 넘어가면 api 를 따로 둬야 하는 거였습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 감사합니당!!👍👍

/**
* 특정 카텟의 정보 반환
*/
@Retryable(retryFor = TooManyRequestException.class, maxAttempts = 3, backoff = @Backoff(delay = 1000))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Retryable 어노테이션을 사용하고 3번의 재시도(maxAttempts)를 하고도 문제가 있으면 어떻게 작동하나요??
TooManyRequestException예외가 던져지는게 맞을까요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3번의 재시도에도 문제가 발생한거면 우리 서비스의 문제보다는
외부 API 서비스의 exception이라고 생각하여 던집니다.
처리 방식이 생각난다면 적용해도 좋을거같아요
하지만 이 에러는 우리 자체에서 발생하는 건 아니여서 저렇게 처리했습니다.

}

/**
* index page 별로 image 반환
Copy link
Collaborator

Choose a reason for hiding this comment

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

index page가 정확히 무엇을 의미하는지 궁금합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

42API는 pagable의 request를 지원하지 않아서 직접 우리가 page를 설정하여, 요청해야합니다.
즉, 1000명의 유저의 url를 받아오는 과정에서 한번에 1000명을 요청하기 보다, 페이지를 나누어서 100명씩 10번에 나눠서 요청합니다.
왜냐하면 42API는 1시간에 요청 수가 정해져 있기 때문입니다. APi Setting에서 설정 값을 확인해 보면 될거같아요

@leeesooha leeesooha requested a review from hani-j October 7, 2024 09:34
@@ -0,0 +1,14 @@
package kr.where.backend.api.json;
Copy link
Collaborator

Choose a reason for hiding this comment

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

OAuthTokenDto가 dto나 oauthtoken 폴더에 있지않고 json폴더에 있는 이유가 궁금합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oauthToken은 서비스 자체에서 생성하거나, 발행해주는 것이 아닌 42api를 통해 발급받는것이여서 이렇게 분류했습니다.
json에는 외부 api 요청 후 응답받는 json value를 Class로 mapping하는 과정에 필요한 dto라고 생각하시면 됩니다

}
memberService.createAgreeMember(
CadetPrivacy
.builder()
Copy link
Collaborator

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.

음 많은이유가 있는데, 개인의 취향이라고 생각하면 편합니다
빌더 패턴을 쓰는 이유는 생성자를 사용시 변수를 지정하지 않으면 null로 채우주는 편리성 때문에 사용하는데
저같은 경우에는 가독성 측면에 좋아보여서 사용합니다.

.builder()
.build(),
haneApiService
.getHaneInfo(member.getIntraName(), TOKEN_HANE)
Copy link
Collaborator

@leeesooha leeesooha Oct 13, 2024

Choose a reason for hiding this comment

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

TOKEN_HANE의 값이 "hane" 로 설정되어있습니다. 그런데 TOKEN_HANE를 쓰는곳을 계속 따라 들어가니 아래 코드가 나왔습니다.

    public static HttpEntity<MultiValueMap<String, String>> requestHaneInfo(final String token) {
        final HttpHeaders headers = new HttpHeaders();

        headers.add(HttpHeaders.AUTHORIZATION, BEARER + token);
        headers.add(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE);

        final MultiValueMap<String, String> params = new LinkedMultiValueMap<>();

        return new HttpEntity<>(params, headers);
    }
    
    
    
위 코드 4번째 줄에서 "BEARER + token"이라고 되있는데 token은 "hane" string이 그냥 들어 가는것 같습니다. 
제 이해로는 "BEARER + 진짜 토큰"을 넣어야 할거은데 "BEARER + hane"가 들어가네요
왜 이렇게 작성된 건지 알수 있을까요?
      

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HANE string이 그냥 들어가지 않고, 디비에서 token 이름이 'hane'인 컬럼을 조회해서 사용합니다

//// session.invalidate();
// return new ResponseEntity(Response.res(StatusCode.OK, ResponseMsg.ADMIN_LOGOUT_SUCCESS), HttpStatus.OK);
// }
//}
Copy link
Collaborator

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.

admin 컨트롤러 사용하지 않아요~

}

public Long getDefaultGroupId() {
if (defaultGroupId != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

defaultGroupId가 널일 경우가 언제인가요?? where42 서비스 동의를 안 한 경우인가요??
그리고 유효할 경우 return 정상, 아닌경우 자연스럽게 예외를 throw하는 방법이 너무 코드가 깔끔하고 좋은 방법이라 생각하는데 이런 플로우를 짜게 되면 일관성 있게 다른 코드들도 이렇게 구현하는게 나을까요??

Copy link
Collaborator

Choose a reason for hiding this comment

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

예를 들어 if 유효하지 않은 경우 {throw}랑 위와 같은 코드랑 고민이 될 때가 있어서 남깁니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultGroup id가 없는경우는 where42 동의를 하지 않은 경우 입니다
-> 비동의 맴버로 남는다면, defaultId 없이 Member 엔티티만 생성 됩니다

음 마지막 질문이 이해가 잘 안되는데 혹시 어떤 부분일까요?
저희 컨벤션은 if (유효한 경우, 유효한 값을 리턴) 그 외 throw 하는 방식을 선택하고 있습니다.
그리고 만약 자료구조를 순회하거나, Optional<>을 사용할때는 람다식으로 orElseThrow()를 사용해서 예외 처리하고 있습니다

컨벤션은 정의하고 팀 끼리 지켜나가면 좋을거같습니다
다른 질문이었다면 편하게 남겨주세용

Copy link
Collaborator

Choose a reason for hiding this comment

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

"저희 컨벤션은 if (유효한 경우, 유효한 값을 리턴) 그 외 throw 하는 방식을 선택하고 있습니다." 로 이해 했습니다 ! 딱 제가 원하던 답변을 주셨어요. 감사합니다.

Optional<> 을 좀 많이 사용해보고 람다식도 얼른 연습해보겠습니다.

if (principle.equals("anonymousUser")) {
throw new AuthUserException.AnonymousUserException();
}
return (AuthUser) principle;
Copy link
Collaborator

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.

저희 v2의 컨벤션이에요

Copy link
Collaborator

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.

음 저희는 만나서 코드 작성해서 정해진건 없지만 네이버를 기준으로 합니다

while (true) {
final List<Cluster> loginMember = intraApiService.getCadetsInCluster(token, page);
result.addAll(loginMember);
if (loginMember.get(99).getEnd_at() != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

42 loginCadet api가 1페이지 당 100개의 요소들이 있는 리스트를 반환하기 때문에 그 페이지의 마지막 요소가 널 일 때까지 루프를 도는건가요..??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

얍 그렇습니다!

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.

코드 리뷰
6 participants