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

Book, Page 등 Entity, Storage, Repository, FileManager 생성 #82

Merged
merged 48 commits into from
Nov 27, 2024

Conversation

iceHood
Copy link
Collaborator

@iceHood iceHood commented Nov 26, 2024

#️⃣ 연관된 이슈


⏰ 작업 시간

예상 시간 실제 걸린 시간
없음 16

📝 작업 내용

  • Entity만들기 (Page, Book, MediaDescription, MediaType)
  • Repository 만들기 (BookRepository, MediaRepository)
  • Storage 만들기 (BookStorage, MHFileManager)
  • CoreData 모델 만들기
    • CoreData 스레드 이슈 해결
  • BookStorageTests
  • 에러 유형 분리 (MHError -> MHCoreError, MHDomainError)
  • Repository, Storage는 Result를 반환하도록 설정

📸 스크린샷

image


📒 리뷰 노트

사실 대부분 영빛님의 코드를 CopyAndPaste한것밖에 없습니다…
모든 Entity들을 CoreData로 사부작 해보려고 했으나 능지이슈로 어떻게 하지도 못하고 Book만 저장을 하려고 했는데 이것도 막혀서 고민입니다…

FileManager의 경우 일단 필요해보이는 부분만 구현했습니다. 저장, 읽기, 삭제, 이동 정도 있으면 될 것같아사… 필요한 기능 있으면 더 말씀주세요! (상상코딩 한거라… 오류가 있을듯…)

...

아... 좀 오래걸렸죠?
보기도 힘드실 겁니다... ㅠㅠ

컨벤션도 엉망인 것같고... 이랬다 저랬다....
정신없이 하느라 뭔가 막한것같네요....


⚽️ 트러블 슈팅

CoreData 설정

커스텀 타입을 코어데이터 모델에 어떻게 저장해야할지,, 고민입니다…
처음에 그냥 BookDTO가 Codable하게 해서JSONEncoder/Decoder를 이용해서 Data로 저장할까 했는데요… 뭔가 이게 맞나? 싶어서 바꿨습니다…

그런데? 아래처럼 하니까 BookDTO가 NSObject가 아니라고 합니다…

image

어떻게 해야할까요… 저에게 능지를…ㅠㅠ

.... 그후....

image
image
image

저는 이것을 관계로 설정하였읍니다...
그리고 이런정보가 있길래 참고하여 DTO - CoreModel 간의 매핑을 설정했습니다.

CoreData의 스레드 이슈

image
image

아니, 테스트를 하다가 보니까 분명 성공을 했는디,,, 갑자기 안된다구????
말이 안되잖아요...

그래서 오기로 여러번 누르니까 됐다가~~ 안됐다가~~를 불규칙적으로 반복한다는 사실을 알았습니다!

그래서 왜 그러냐.....하다가

이것이 스레드 이슈 때문인 것을 알았습니다...
비동기적으로 접근하다보니 데이터를 읽으려다가 지워진다든지... 하는 상황이 발생하나 봅니다.

그래서 이것을 어떻게 해결하느냐??

아무따 공식문서... 보니까?

When the feed download finishes, the sample uses the task context to consume the feed in the background. In Core Data, every queue-based context has its own serial queue, and apps must serialize the tasks that manipulate the context with the queue by wrapping the code with a perform(_:) — with or without the await keyword — or performAndWait(_:) closure.

이런 내용이 있더라구요...
아하! 그렇구나~~ context처럼 queue based를 쓸때는 perform을 써라~~

그래서 무지성 perform으로 감쌌습니다....

@iceHood iceHood added the ✨ Feature 기능 관련 작업 label Nov 26, 2024
@iceHood iceHood added this to the 0.4 milestone Nov 26, 2024
@iceHood iceHood self-assigned this Nov 26, 2024
@iceHood iceHood linked an issue Nov 26, 2024 that may be closed by this pull request
Copy link
Collaborator

@k2645 k2645 left a comment

Choose a reason for hiding this comment

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

FileManager 구축 최고최고 ..🥹

Comment on lines 9 to 12
case fileCreationError
case fileReadingError
case fileDeletionError
case fileMovingError
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: description을 보니 실패와 관련된 에러같은데 위 에러들과 통일성을 주어 Failure은 어떤가욥?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

인정합니다ㅂ! 반영완

Comment on lines 32 to 34
request.predicate = NSPredicate(
format: "id LIKE %@", "\(id.uuidString)"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

옹 Predicate 잘 되나요..?? 테스트 코드 작성해서 해당 부분이 잘 돌아가는지 확인해보는것도 좋을 것 같네욥

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

안돼서... 되게 바꿨습니다 ㅋㅋ

}

extension CoreDataBookStorage: BookStorage {
func create(data: BookDTO) async -> Result<Void, MHCore.MHError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2: 아래도 마찬가지인데 Result의 MHError 타입에 MHCore는 제거해주면 좋을 것 같아용

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

자동완성의 폐혜... 감사합니다! 반영했어욤

Comment on lines 4 to 7
func create(book: Book) async
func fetchBook(with id: UUID) async -> Book?
func update(id: UUID, book: Book) async
func deleteBook(_ id: UUID) async
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: 어떤 메서드에는 book이 붙고 어떤 메서드에는 Book이 안붙어서 어색한것 같아요.. 근데 방금 찾아보니 제가 BookCover 만들때도 저렇게 해놨네요 헷 ^0^ 하나로 통일하면 좋을 것 같은데 어떻게 생각하시나욥??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

엌ㅋㅋㅋ 복붙의 흔적이 또... 하나로 통일 좋습니다! 그냥 book을 빼버릴까요? BookRepository니까>..

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 +4 to +5
public let id: UUID
public let type: MediaType
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2: 이미지의 경우는 캡션을 담을 description이랑 날짜를 담을 data타입의 attribute도 필요할 것 같은데 어떻게하면 좋을까요...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그것은....약간 생각난게
1.

 struct MHImage: Codable { 
   날짜: Date, 설명: String, 이미지데이터: Data }

이렇게 하는 방법이 있을 것같구. (맞는지는 모르겠습니다.)
2.

struct MediaDescription {
 ...
 let attribute: [String: Any] // 각자 필요한거 넣기
}

이것도 맞는지는 모르겠음요...
당장 떠오르는 방법은 이것들이네욤,, 혹시 좋은 방안 있으신가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

제 생각이 제ㅔ일 구릴것같긴한데

struct MediaDescription {
 ...
 let imageCaption: String?
let creationDate: Date?
}

이렇게 옵셔널로 두고 mediaType이 image일 때에만..저기에 값..을..넣어준다...?
근데 넘 구리네용 ^0^.....

정현님 2번째 방법도 전 좋을 것 같아요....ᐟ.ᐟ

Copy link
Collaborator

@yuncheol-AHN yuncheol-AHN 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 12 to 16
public func create(book: Book) async {
let bookDTO = mappingBookToDTO(book)

_ = await storage.create(data: bookDTO)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

질문
해당 코드를 포함한 create, update, delete 메서드에서 Result<,> 처리는 어느 부분에서 해주나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

사실 있는 코드 복붙한거라 생각 안하고 하긴했습니다 ㅋㅋㅋ
이거 result로 return해도 될 것같긴합니다.
@k2645 님! 혹시 Result로 반환 안하신 이유가 있으신가요?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

엇 저도 실은.. 크게..생각을 안하고 있었던 것 같...숩다.......... UseCase도 그럼 Result로 다..넘겨줘야할까오/...? 아님 Repository부터는 throw를 해도 될 것 같기도하구요/...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Resut = Storage, Repository
throws = ViewModel, UseCase

이렇게 하면 되겠죠 ?!

Copy link
Collaborator

@Kyxxn Kyxxn 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 9 to 14
case directorySettingFailure
case fileCreationFailure
case fileReadingFailure
case fileDeletionFailure
case fileMovingFailure

Copy link
Collaborator

Choose a reason for hiding this comment

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

P2 질문
파일과 관련된 에러처리 열거형은 따로 정의를 해두는 건 어떨까요 ?
모든 에러를 여기서 감당하면 파일의 길이가 길어질 거 같은데 괜찮을까요 ?!

Copy link
Collaborator 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.

전 모듈별로 에러를 만들어서 처리하는거 좋다고 생각합니다 .ᐟ.ᐟ

Copy link
Collaborator

Choose a reason for hiding this comment

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

임정합니더 오히려 좋을 거 같아요

public struct BookDTO {
let id: UUID
let pages: [PageDTO]

Copy link
Collaborator

Choose a reason for hiding this comment

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

public init 없이 잘 동작하나요 ??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Internal하게 사용돼서 아직 오류가 안나온 것같습니다! 혹시 모르니 넣어 놓을께욤!

}

// MARK: - DTO to Core
private func DTOPagesToCore(_ pages: [PageDTO]) -> Data? {
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 Author

Choose a reason for hiding this comment

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

하하.. 이거 함 갈아엎었읍니다..

Comment on lines 12 to 16
public func create(book: Book) async {
let bookDTO = mappingBookToDTO(book)

_ = await storage.create(data: bookDTO)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Resut = Storage, Repository
throws = ViewModel, UseCase

이렇게 하면 되겠죠 ?!

@testable import MHData
@testable import MHFoundation

@Suite("Serial model tests", .serialized) final class MHFileManagerTests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suite는 어떤 거죠..?!

Copy link
Collaborator Author

@iceHood iceHood Nov 27, 2024

Choose a reason for hiding this comment

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

일종의 테스트 모음이라고 합니다.
암시적으로 test가 포함된 객체는 테스트 모음인데,
저걸 쓰면 명시적으로 테스트 모음이라고 선언하고 필요한 추가 기능을 붙일 수 있다고 핮니다
저는 순차적으로 동작해야해서 serialization을 추가했습니다.
참고한자료

Copy link
Collaborator

Choose a reason for hiding this comment

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

오 감사합니다 !!

@iceHood iceHood marked this pull request as ready for review November 27, 2024 14:20
Copy link
Collaborator

@k2645 k2645 left a comment

Choose a reason for hiding this comment

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

와웅...정말 정현님이...다 하셨군요...🥹 너무너무너무 수고많으셨습니다 🥹 역시..역시...

혹시 PR description에 나온 CoreDataModel 데이터 사진 중 coredata의 relation이 어떤 것은 O? 이고 어떤것은 M 이던데, 이 둘의 차이가 무엇인지 알 수 있을까욥?? 약간 포함 상하? 관계?? 그런건가요??

Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: 해당 에러는 DataLayer에서만 사용되는 Error인 것 같은데, 그냥 DataLayer에 두고 사용하면 되지 않나요?? 혹시 Core 모듈에 두신 이유가 있으신가욤??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그게... 사실 DataLayer에 두고 싶었습니다...
그렇게 하니까 Domain이 DataLayer에 의존해야하더라구요...
그래서 어떻게 하지... 하다가 그냥 파일로만 나누자... 했습니다 ㅠㅠ

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하...그런 문제가.... 맞네용.. 정현님의 판단이 1928478번 정확한 판단인 것 같습니다

Comment on lines +17 to +26
try await context.perform { [weak self] in
guard let self else { return }
guard let entity = NSEntityDescription.entity(forEntityName: "BookEntity", in: context) else {
throw MHDataError.noSuchEntity(key: "BookEntity")
}
let book = NSManagedObject(entity: entity, insertInto: context)
book.setValue(data.id, forKey: "id")
book.setValue(dtoPagesToCore(data.pages), forKey: "pages")
try context.save()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 context.perform으로 묶어주신 이유가 있으신가요?? 이렇게하면 어떤점이 달라지는지 궁금합니다 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 PR description을 뒤늦게 읽었네요... 오홍.. 스레드 문제라.. 싱기하군여

Comment on lines +118 to +164
// MARK: - Core to DTO
private func coreBookToDTO(_ book: BookEntity) -> BookDTO? {
guard let id = book.id,
let corePages = book.pages
else { return nil }
let pages = corePagesToDTO(corePages)

return BookDTO(
id: id,
pages: pages
)
}
private func corePagesToDTO(_ pages: NSOrderedSet) -> [PageDTO] {
return pages.compactMap {
guard let page = $0 as? PageEntity else { return nil }
return corePageToDTO(page)
}
}
private func corePageToDTO(_ page: PageEntity) -> PageDTO? {
guard let id = page.id,
let mediaDescriptions = page.mediaDescriptions,
let text = page.text
else { return nil }

let metadata = mediaDescriptions.reduce(into: [Int: MediaDescriptionDTO]()) { partialResult, element in
guard let element = element as? MediaDescriptionEntity else { return }
guard let (index, mediaDescription) = coreMediaDescriptionToDTO(element) else { return }
partialResult[index] = mediaDescription
}

return PageDTO(
id: id,
metadata: metadata,
text: text
)
}
private func coreMediaDescriptionToDTO(_ mediaDescription: MediaDescriptionEntity) -> (Int, MediaDescriptionDTO)? {
guard let id = mediaDescription.id,
let type = mediaDescription.type
else { return nil }
let location = mediaDescription.location
return (Int(location), MediaDescriptionDTO(
id: id,
type: type,
attributes: mediaDescription.attributes
))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3: 저는 Entity를 DTO로 바꾸는 로직을 해당 Entity클래스의 extension으로 구현해주었는데 이렇게 구현할 수도 있었군요 .ᐟ.ᐟ BookCoverEntity도 통일시켜서 위와같이 구현하면 좋을 것 같네용

Copy link
Collaborator 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.

캬.. 빛 정 현.. 꼼꼼한 주석 또 한 번 제 이마를 탁치고 배우고 갑니다..

Copy link
Collaborator

Choose a reason for hiding this comment

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

ㅘㅜ 주석 깔꼼하네요

@iceHood
Copy link
Collaborator Author

iceHood commented Nov 27, 2024

와웅...정말 정현님이...다 하셨군요...🥹 너무너무너무 수고많으셨습니다 🥹 역시..역시...

혹시 PR description에 나온 CoreDataModel 데이터 사진 중 coredata의 relation이 어떤 것은 O? 이고 어떤것은 M 이던데, 이 둘의 차이가 무엇인지 알 수 있을까욥?? 약간 포함 상하? 관계?? 그런건가요??

음 뭔가 Many의 Ⓜ️ One의 🅾️ 인것같습니다 ㅋㅋㅋㅋㅋ
Book : Page이 1 : N, Page : Description이 다시 1 : N 관계입니다!

Copy link
Collaborator

@Kyxxn Kyxxn left a comment

Choose a reason for hiding this comment

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

�1400줄의 코드를 완벽하게 적어내는 사나이..
그 이름 Steve Lim..!

@iceHood iceHood merged commit f33f745 into develop Nov 27, 2024
2 checks passed
@iceHood iceHood deleted the feature/creat-book-page-domain branch November 27, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 관련 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

필요한 Entity, Repository 만들기
4 participants