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

#80 [REFACTOR] 표정 인식 API 리팩토링 및 미션 수행 API 통합 #110

Merged
merged 24 commits into from
Mar 28, 2024
Merged

#80 [REFACTOR] 표정 인식 API 리팩토링 및 미션 수행 API 통합 #110

merged 24 commits into from
Mar 28, 2024

Conversation

05AM
Copy link
Member

@05AM 05AM commented Mar 27, 2024

☀️ 작업 사항

  • CustomAnalysisEntity, VisionAnalysisResult 등의 클래스 구조 개선
  • 표정 인식 API를 통일된 부모 클래스를 사용하여 구현
  • 미션 수행 API를 하나로 통일 ( 직접촬영, 객체탐지, 표정인식 )
  • 외부 이미지 분석 서비스를 인터페이스로 구현하여 요청 값에 따라 동적으로 수행하도록 구현
  • 미션 이미지 전략을 구현하여 요청의 미션 값에 따라 동적으로 분석을 선택할 수 있도록 구현
  • Image 관련 Util 클래스 리팩토링
    • ImageUtil
    • ImageProcessor
    • ImageDrawer
    • 해당 부분은 아직 완전하게 마치지 못했습니다. 추후 보완해서 pr 올리겠습니다.
  • 외부 이미지 분석 서비스 파일 크기 요청 제약을 만족시키기 위해 특정 크기가 될 때까지 압축하는 기능 구현
  • API 명세서 설명 추가
  • 사용하지 않는 모델로 분석하는 코드 삭제
  • 분석 결과를 그리는 기능 세부 조정
  • 클래스 응집도를 높이기 위해 패키지 이전

☀️ 관련 이슈

resolved : #80

☀️ 참고 사항

  • 나눠서 올렸어야 했는데 이전 코드가 의존성이 커서 연쇄적으로 수정해야 하여 한번에 많은 양의 수정을 PR 올린 것을 양해 부탁 드립니다! 😥
  • 패키지 이전한 코드는 거의 보실 필요가 없다고 생각되고, 커밋 별로 보시는 것이 편할 것 같습니다!
  • 전에 올리신 pr은 내일 중으로 리뷰하겠습니다!

05AM added 24 commits March 26, 2024 13:01
 - 판단하는 비즈니스 로직을 포함하고 있어 이전하였습니다.
  - 부모 클래스 MissionPerformResult 삭제
  - 필드 targetName 추가
 - ImageUtil: 일반적인 기능 처리 (이미지 파일 읽기, 메타 데이터 가져오기 등)
 - ImageDrawer: 이미지 위에 그림 그리기
 - ImageProcessor: 압축, 리사이징 등 이미지 전체 관련 처리
 - VisionServiceFactory을 구현하여 동적으로 분석 서비스를 반환하는 기능 구현
@05AM 05AM added BUG Something isn't working REFACTOR refactored code D-3 3일 전 까지 리뷰해주세요 labels Mar 27, 2024
@05AM 05AM requested a review from hye-on March 27, 2024 16:10
@05AM 05AM self-assigned this Mar 27, 2024
Copy link
Contributor

@hye-on hye-on left a comment

Choose a reason for hiding this comment

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

ai를 다루는 부분이 변수가 많아서 수정사항이 많이 생기네요 😢
리팩토링덕분에 예전보다 코드 이해가 훨씬 잘되고, 배워가는 부분도 많네요
고생하셨습니다!

return orientByteImage(data, metadata, contentType);
}

private static byte[] compress(byte[] imageByte, float quality, ImageContentType contentType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

api 압축 조건이 어떻게 되나요??
클라이언트에서 압축해서 보내는데
조건에 부합하지 않을 때가 있어서 압축하는 건가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

클라이언트에서는 압축해서 보내지 않고 서버에서 압축하고 있습니다.
추후에 이미지 압축, 결과 그리기를 클라이언트에서 구현하는 것을 요청할 예정입니다.

Azure Vision Service v3.2의 경우 이미지 제한이 4MB이고, 이미지가 너무 크면 요청 시간이 걸리기 때문에 압축을 적용했습니다.

그런데 서버에서 시간이 좀 오래 걸려서 이미지 크기 제한이 명시되어 있지 않은 v4.0을 사용할까 고민중입니다.

그런데 3.2에서는 parent 필드를 제공해주기 떄문에 v4.0보다 그림틀이 그려지는 경우가 더 있어서 어느 버전을 사용해야 할지 고민중입니다.

지금은 일단 4.0 버전을 사용하다가 클라이언트에 압축 로직이 구현되면 3.2로 다시 바꿀까 생각중입니다.
서비스만 교체해주면 되기 때문에 코드의 변경은 없을 예정입니다.

Copy link
Contributor

@hye-on hye-on Mar 29, 2024

Choose a reason for hiding this comment

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

클라이언트에서는 압축해서 보내지 않고 서버에서 압축하고 있습니다.
추후에 이미지 압축, 결과 그리기를 클라이언트에서 구현하는 것을 요청할 예정입니다.

전에 압축해서 보내고 있다고 들어서 클라이언트에 여쭤봤는데
지금 4MB로 압축하고 보내고 있고 찬미님이 압축하는 부분 코드 빼도 된다고 하셨다고 들었어요!

클라이언트에서 압축하다가 , 지금은 서버에서 압축하고 , 추후에 클라이언트에서 압축하는 것으로 바꾸는 것으로 결정하신 이유가 궁금합니다!

(추가적인 로직 때문에 그런건지, 테스트를 위해 일시적으로 서버에 압축하는 건지, 제가 몰랐던 제약 사항이 있는건지 등등)

Copy link
Member Author

Choose a reason for hiding this comment

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

서버 환경에서 직접 테스트해보다가 크기가 일정 이상인 경우에 에러가 발생해서 취한 조치였는데 클라이언트에서 압축을 구현한 걸 잊고 있었네요!

압축 로직을 제거하고 요청에서 크기 제한을 하는 것으로 수정하겠습니다!

import org.springframework.web.multipart.MultipartFile;

@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class ImageUtil {
Copy link
Contributor

Choose a reason for hiding this comment

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

p5 : 이미지유틸하고 이미지프로세서하고 이름만 봤을때는 비슷해보이기도 하는데
이미지 유틸이 현재 읽어오는 기능을 하는 메소드만 있는 것 같아서
이미지 유틸을 이미지로더, 이미지패처같은 이름으로 수정하는 것이 어떨까요?

커밋 내용에 "- ImageUtil : 일반적인 기능 처리" 라고 되어있는데 이미지유틸이 읽어오는 기능뿐만이 아니라 다른 기능을 하는 메소드가 추가되는 것을 고려한 이름이였다면,
util, processor 가 어떤 기준으로 구분되는지 궁금합니다!

Copy link
Member Author

@05AM 05AM Mar 28, 2024

Choose a reason for hiding this comment

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

이미지를 읽어오기도 하고 이미지의 메타 데이터도 가져오는 역할을 하고 있습니다.
또한 이후로도 이미지 유틸 클래스들에 필요한 공통 메서드나 일반적으로 사용되는 기능을 구현할 예정이고,
ImageProcessor와 ImageDrawer의 부모이기도 합니다.

전부 Image와 관련된 Util 클래스로 부모 클래스가 ImageUtil인 것은 개인적으로 적합한 이름인 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

상속관계를 못봤네요! 확인했습니다!

try {
return file.getBytes();
} catch (IOException e) {
throw new BaseException("파일 읽기를 실패했습니다.", e, Status.INVALID_IMAGE);
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
Member Author

Choose a reason for hiding this comment

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

해당 부분에만 사용되는 에러 메시지기도 하고 로그에 적을 추가적인 정보를 생성자에 제공했습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 그래서 BaseException도 같이 수정한 거군요 확인했습니다!

Comment on lines +44 to +47
while (data.length >= sizeLimit && cnt++ <= 3) {
data = compress(data, 0.5f, contentType);
System.out.println(data.length);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

p5 : 이분탐색을 통해 최적의 quality(압축품질)를 찾는 것도 좋을 것 같아요!
시간 복잡도가 O(log n)이라서 빠르게 찾을 수 있을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

오 좋은 정보 감사합니다! 추후 시도해보겠습니다.

Comment on lines +14 to +23

public BaseException(String message, Throwable cause, Status status) {
super(message, cause);
this.status = status;
}

public BaseException(Throwable cause, Status status) {
super(cause);
this.status = status;
}
Copy link
Contributor

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 +33
public class VisionServiceFactory {

private final AzureVision32Service azureVision32Service;
private final AzureVision40Service azureVision40Service;
private final GoogleVisionService googleVisionService;

public VisionService getVisionService(MissionType missionType) {
switch (missionType) {
case OBJECT_DETECTION:
return azureVision40Service;

case EXPRESSION_RECOGNITION:
return googleVisionService;

default:
throw new BaseException(Status.INTERNAL_SERVER_ERROR);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +45 to +58

public void validateMissionObject(MissionType type, String object) {
if (object == null) {
throw new BaseException(Status.BAD_REQUEST_PARAM);
}

if (isNotExistMissionObjectName(type.getName(), object)) {
throw new BaseException(Status.MISSION_NOT_FOUND);
}
}

private boolean isNotExistMissionObjectName(String missionName, String object) {
return !missionObjectRepository.existsByMission_NameAndName(missionName, object);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

미션 프로바이더 역할이 궁금해요!
뭔가를 제공한다는 뜻이거나 cqrs패턴으로 사용하는 거라면,
validateMissionObject 반환값이 void인데 이 메소드를 여기에 배치한 이유가 궁금합니다!

Copy link
Member Author

@05AM 05AM Mar 28, 2024

Choose a reason for hiding this comment

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

해당 부분은 별 의미 없이 구현했습니다. provider를 service로 합치려고 생각 중인데, 아직 Mission 도메인의 클래스들은 리팩토링을 못해서 추후 반영하겠습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 찬미님이 하신 대로 service -provider가 역할을 나누는데 좋다고 생각해서 service, provider로 나누려고 했었거든요
(cqrs패턴의 개념으로)
다음 회의 때 service- provider로 나눌지, 나눈다면 역할을 어떻게 할지 논의해보면 좋을 것 같습니다!

@05AM
Copy link
Member Author

05AM commented Mar 28, 2024

@hye-on

우선 approve하신걸로 알고 머지하겠습니다!

@05AM 05AM merged commit 95b5e30 into Wake-up-together-TogetUp:main Mar 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Something isn't working D-3 3일 전 까지 리뷰해주세요 REFACTOR refactored code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 미션 판독 api migration
2 participants