feat!: MemberId/EventId/ExhibitIdをBranded Typeに変更#92
Conversation
KarteIdパターンに倣い、Member/Event/Exhibitの各IDをstring型からBranded Typeに置換。 コンパイル時にID種別の混同を防止し、型安全性を向上させる。 BREAKING CHANGE: 全集約ルートID(MemberId, EventId)およびExhibitIdの型がstringからBranded Typeに変更。 ファクトリ関数 memberId(), eventId(), exhibitId() を通じてIDを生成する必要がある。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Exhibit.tsの意図説明コメント(NOTE)の復元 - DrizzleEventRepositoryのパラメータ名を元の命名に復元(evtId→eventId等) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Member/Event/Exhibit の各IDを string から Branded Type に移行し、異なるID種別の取り違えをコンパイル時に防げるようにする変更です(既存の KarteId パターンに合わせた型安全化)。
Changes:
MemberId/EventId/ExhibitIdの Branded Type とファクトリ関数を追加- ドメイン層(集約・エンティティ・リポジトリI/F)とアプリ層(UseCase Input)のID型をBranded Typeへ置換
- インフラ層(Drizzle)のDB↔ドメイン変換境界でファクトリ関数によるキャストを追加し、テストデータも追従
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/domain/aggregates/karte/Karte.test.ts | assignedMemberIds を MemberId 生成関数経由に更新 |
| src/infrastructure/drizzle/DrizzleMemberRepository.ts | DB→ドメイン変換とRepository I/Fを MemberId に対応 |
| src/infrastructure/drizzle/DrizzleEventRepository.ts | DB→ドメイン変換・検索/削除APIを EventId/ExhibitId/MemberId に対応 |
| src/domain/aggregates/member/index.ts | MemberId をドメイン集約の公開APIに追加 |
| src/domain/aggregates/member/MemberRepository.ts | Repository I/F のID型を MemberId に変更 |
| src/domain/aggregates/member/MemberId.ts | MemberId Branded Type とファクトリを追加 |
| src/domain/aggregates/member/Member.ts | Member エンティティの id を MemberId 化 |
| src/domain/aggregates/member/DiscordAccount.ts | DiscordAccount の memberId を MemberId 化 |
| src/domain/aggregates/karte/SupportRecord.ts | assignedMemberIds を MemberId の NonEmptyArray に変更 |
| src/domain/aggregates/karte/Karte.ts | Karte作成Props内の assignedMemberIds を MemberId に変更 |
| src/domain/aggregates/event/index.ts | EventId/ExhibitId をドメイン集約の公開APIに追加 |
| src/domain/aggregates/event/LightningTalk.ts | LightningTalk の exhibitId を ExhibitId 化 |
| src/domain/aggregates/event/ExhibitId.ts | ExhibitId Branded Type とファクトリを追加 |
| src/domain/aggregates/event/Exhibit.ts | Exhibit の id / memberIds を ExhibitId/MemberId 化 |
| src/domain/aggregates/event/EventRepository.ts | Repository I/F のID型を EventId/ExhibitId/MemberId に変更 |
| src/domain/aggregates/event/EventId.ts | EventId Branded Type とファクトリを追加 |
| src/domain/aggregates/event/Event.ts | Event の id / memberIds / exhibit操作API引数を Branded Type 化 |
| src/application/usecase/member/UpdateMember.ts | UseCase Input の memberId を MemberId 化 |
| src/application/usecase/member/RegisterMember.ts | 新規Member生成時に memberId(uuid()) を使用 |
| src/application/usecase/member/GetMember.ts | UseCase Input の id を MemberId 化 |
| src/application/usecase/member/ConnectDiscordAccount.ts | UseCase Input の memberId を MemberId 化 |
| src/application/usecase/eventParticipation/RemoveMemberFromExhibit.ts | UseCase Input を MemberId/ExhibitId 化 |
| src/application/usecase/eventParticipation/RemoveMemberFromEvent.ts | UseCase Input を MemberId/EventId 化 |
| src/application/usecase/eventParticipation/RegisterMemberToExhibit.ts | UseCase Input を MemberId/ExhibitId 化 |
| src/application/usecase/eventParticipation/RegisterMemberToEvent.ts | UseCase Input を MemberId/EventId 化 |
| src/application/usecase/eventParticipation/GetMembersByExhibit.ts | UseCase Input を ExhibitId 化 |
| src/application/usecase/eventParticipation/GetMembersByEvent.ts | UseCase Input を EventId 化 |
| src/application/usecase/eventParticipation/GetExhibitsByMember.ts | UseCase Input を MemberId 化 |
| src/application/usecase/eventParticipation/GetEventsByMember.ts | UseCase Input を MemberId 化 |
| src/application/usecase/event/UpdateEvent.ts | UseCase Input の eventId を EventId 化 |
| src/application/usecase/event/RemoveExhibitFromEvent.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/GetEvent.ts | UseCase Input の eventId を EventId 化 |
| src/application/usecase/event/DeleteEvent.ts | UseCase Input の eventId を EventId 化 |
| src/application/usecase/event/CreateEvent.ts | UseCase Input の id を EventId 化 |
| src/application/usecase/event/ChangeLightningTalkStartTime.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/ChangeLightningTalkSlideUrl.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/ChangeLightningTalkDuration.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/ChangeExhibitUrl.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/ChangeExhibitName.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/ChangeExhibitMarkdownContent.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/ChangeExhibitDescription.ts | UseCase Input を EventId/ExhibitId 化 |
| src/application/usecase/event/AddExhibitToEvent.ts | UseCase Input を EventId/ExhibitId 化 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const exhibit = new Exhibit(id, name, description, markdownContent, url); | ||
| // NOTE: ここで例外を投げるのは、Exhibit の id と LightningTalk の exhibitId が一致しているかどうかをチェックしているため | ||
| if (lt.exhibitId !== id) { | ||
| throw new LightningTalkExhibitIdMismatchException(id, lt.exhibitId); |
There was a problem hiding this comment.
LightningTalkExhibitIdMismatchException のコンストラクタは (lightningTalkId, exhibitId) の順ですが、ここでは id(Exhibit側) を第1引数、lt.exhibitId を第2引数に渡しているため、例外メッセージ内のIDが入れ替わって表示されます。lt.exhibitId を第1引数、id を第2引数にするなど、メッセージの意図に合う順序で渡してください。
| throw new LightningTalkExhibitIdMismatchException(id, lt.exhibitId); | |
| throw new LightningTalkExhibitIdMismatchException(lt.exhibitId, id); |
There was a problem hiding this comment.
🟡 EventNotFoundException receives ExhibitId instead of EventId, producing misleading error message
RegisterMemberToExhibit passes input.exhibitId (typed ExhibitId) to EventNotFoundException at line 39, which formats the message as イベントが見つかりません: <exhibitId> (src/application/exceptions/ApplicationExceptions.ts:9-10). This labels an exhibit ID as an event ID in the error output, making debugging harder. This is a pre-existing issue now made more visible by the PR's introduction of branded types to distinguish ID kinds — the very class of bug this PR aims to prevent. The same pattern exists in RemoveMemberFromExhibit.ts:36.
(Refers to line 39)
Prompt for agents
In both src/application/usecase/eventParticipation/RegisterMemberToExhibit.ts (line 39) and src/application/usecase/eventParticipation/RemoveMemberFromExhibit.ts (line 36), the code throws EventNotFoundException(input.exhibitId) when findByExhibitId returns null. This passes an ExhibitId to an exception that semantically expects an EventId, producing the misleading message 'イベントが見つかりません: <exhibitId>'. Consider either: (1) using ExhibitNotFoundFromExhibitIdException(input.exhibitId) instead, which already exists and produces the correct message, or (2) creating a dedicated exception for this case like 'EventNotFoundForExhibitException'.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🟡 EventNotFoundException receives ExhibitId instead of EventId in RemoveMemberFromExhibit
Same issue as in RegisterMemberToExhibit: RemoveMemberFromExhibit passes input.exhibitId (typed ExhibitId) to EventNotFoundException at line 36, producing the misleading error message イベントが見つかりません: <exhibitId>. The ExhibitNotFoundFromExhibitIdException (src/application/exceptions/ApplicationExceptions.ts:24-29) already exists and would be more appropriate here.
(Refers to line 36)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🚩 GetMembersByExhibit retrieves event-level members instead of exhibit-level members
The GetMembersByExhibit use case at src/application/usecase/eventParticipation/GetMembersByExhibit.ts:37-39 calls event.getMemberIds() which returns all member IDs of the event, not the specific exhibit's members. Given the use case name 'GetMembersByExhibit', it should likely find the specific exhibit within the event and call exhibit.getMemberIds() instead. This is a pre-existing logic issue not introduced by this PR, but worth noting as the branded types now make the distinction between event-level and exhibit-level membership clearer.
(Refers to lines 36-41)
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -0,0 +1,6 @@ | |||
| /** イベントID — Event集約ルートを一意に識別するブランド型 */ | |||
| export type EventId = string & { readonly __brand: unique symbol }; | |||
There was a problem hiding this comment.
📝 Info: Branded type __brand uses identical pattern across all three types — relies on unique symbol per-file
All three branded types (EventId, ExhibitId, MemberId) use the same pattern string & { readonly __brand: unique symbol }. Their mutual incompatibility depends on TypeScript treating each unique symbol declaration in a separate type alias as distinct. This is correct TypeScript behavior — each type alias location creates a new unique symbol type — so these are properly incompatible with each other at the type level. No issue here, but worth noting this is a subtle TypeScript feature that the design relies on.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| // Load exhibit member IDs | ||
| for (const memberExhibit of exhibitRecord.memberExhibits) { | ||
| exhibit.addMemberId(memberExhibit.memberId); | ||
| exhibit.addMemberId(memberId(memberExhibit.memberId)); | ||
| } | ||
|
|
||
| event.addExhibit(exhibit); |
There was a problem hiding this comment.
📝 Info: toDomain reconstruction adds exhibit member IDs to event twice
In DrizzleEventRepository.toDomain() at src/infrastructure/drizzle/DrizzleEventRepository.ts:57-71, event member IDs are first loaded from memberEvents records (line 58-60), then event.addExhibit(exhibit) at line 71 internally re-adds each exhibit's member IDs to the event's memberIds Set (see src/domain/aggregates/event/Event.ts:56-57). This is harmless because Set.add() is idempotent, but it means the reconstruction path duplicates work. Not a bug, just a minor inefficiency.
(Refers to lines 57-71)
Was this helpful? React with 👍 or 👎 to provide feedback.
## Why 集約ルートのID(Member, Event)および子エンティティのID(Exhibit)が`string`型であり、異なるID種別の取り違えがコンパイル時に検出できなかった。既存のKarteIdパターンに合わせてBranded Typeを導入し、型安全性を確保する。 ## What - `MemberId`, `EventId`, `ExhibitId` のBranded Type定義とファクトリ関数を追加 - ドメイン層(集約ルート、エンティティ、リポジトリインターフェース、値オブジェクト)の全ID参照をBranded Typeに変更 - アプリケーション層(全ユースケースのInput型)をBranded Typeに変更 - インフラ層(DrizzleRepository)のDB↔ドメイン変換境界でファクトリ関数によるキャストを追加 - テストコードのassignedMemberIdsをMemberIdに更新 ## BREAKING CHANGE 全集約ルートID(MemberId, EventId)およびExhibitIdの型が`string`からBranded Typeに変更。 呼び出し側は`memberId()`, `eventId()`, `exhibitId()`ファクトリ関数でIDを生成する必要がある。 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/su-its/core/pull/92" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Why
集約ルートのID(Member, Event)および子エンティティのID(Exhibit)が
string型であり、異なるID種別の取り違えがコンパイル時に検出できなかった。既存のKarteIdパターンに合わせてBranded Typeを導入し、型安全性を確保する。What
MemberId,EventId,ExhibitIdのBranded Type定義とファクトリ関数を追加BREAKING CHANGE
全集約ルートID(MemberId, EventId)およびExhibitIdの型が
stringからBranded Typeに変更。呼び出し側は
memberId(),eventId(),exhibitId()ファクトリ関数でIDを生成する必要がある。🤖 Generated with Claude Code