feat: derive media storage paths via hash fallback #409
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Replace direct payload.create calls with upsertByUniqueField(payload, 'posts', 'slug', ...) to make seeding idempotent - Build post data objects first (p1Data, p2Data, p3Data) and upsert them - Use returned p?.doc.id when linking relatedPosts; preserve disableRevalidate context
- Rename media target key from `filename` to `label` - Query `platformContentMedia` by the explicit `alt` field (uses `t.label`) - Set created media `alt` to the label to avoid relying on implicit upload filename values
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces deterministic hash-based media storage paths to replace document ID-based keys, ensuring stable and predictable storage paths for media files. The changes also refactor seeding logic to improve reliability and maintainability.
- Replaced document ID-based storage keys with SHA-1 hash-based deterministic keys for doctor and platform media
- Updated seeding logic to use numeric uploader IDs and improved error handling with fallbacks
- Enhanced demo seeding with better media creation helpers and added validation support
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/hooks/media/computeStorage.ts |
Added hash-based fallback for storage key generation when document ID unavailable |
src/collections/DoctorMedia/hooks/beforeChangeDoctorMedia.ts |
Replaced document ID with deterministic hash for doctor media storage paths |
src/collections/PlatformContentMedia/hooks/beforeChangePlatformContentMedia.ts |
Implemented hash-based storage path generation for platform content media |
src/endpoints/seed/demo/index.ts |
Updated demo seeding with numeric uploader IDs and createMediaFromURL helper |
src/endpoints/seed/clinics/clinics-seed.ts |
Converted uploader ID to number type for consistency |
src/endpoints/seed/clinics/doctors-seed.ts |
Temporarily disabled doctor media seeding and improved logging |
src/endpoints/seed/clinics/treatments-seed.ts |
Added fallback specialty assignment to prevent undefined values |
src/endpoints/seed/posts/posts-seed.ts |
Refactored to use upsertByUniqueField helper for better idempotency |
src/endpoints/seed/posts/post-1.ts |
Replaced code block with generic text paragraph for clarity |
src/collections/PlatformContentMedia/hooks/beforeChangePlatformContentMedia.ts
Outdated
Show resolved
Hide resolved
…aths
- Add vi.mock('crypto') in platformContentMediaBeforeChange and doctorMediaBeforeChange tests to produce stable short-hash values
- Update expected filename and storagePath assertions to use the mocked hash prefix (first 10 chars)
- Adjust comments to clarify hashing expectations in assertions
Coverage Report for Integration Tests
File Coverage
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…omputeStorage - Replace duplicated per-collection hashing/filename logic with computeStorage: - DoctorMedia and PlatformContentMedia now call computeStorage for filename/storagePath. - Remove legacy beforeChangeUserProfileMedia and switch UserProfileMedia to use beforeChangeComputeStorage with ownership freeze and createdBy stamping. - Extend computeStorage: - Add key.type = 'hash' to derive short deterministic folder keys from owner, filename, and optional file size. - Add ownerRequired option to allow owner-less collections (e.g., platform assets). - Improve keySource reporting and logging formatting. - Tests: mock crypto for stable hashes; add tests for hash key derivation and ownerRequired=false; update user profile hook tests to reflect new hash-based behavior. - Docs: clarify media ownership/access and storage path pattern in docs/integrations/storage.md.
…action - docs: update storage.md to use `platform` as the top-level namespace/S3 prefix instead of `media` - hooks: computeStorage imports extractFileSizeFromRequest, derives/sets a keySource, and uses the helper for hash/fallback folder-key derivation; keySource included in telemetry - util: add extractFileSizeFromRequest to normalize file size extraction from varied request shapes - seed: remove unused uploaderId from seedDoctors and calling demo runner; minor formatting cleanup in doctors seed - test: move stable crypto mock in beforeChangeComputeStorage tests to top-level to avoid duplicate mocks and adjust import order
Comment on lines
+41
to
+48
| // Platform (no owner segment) | ||
| beforeChangeComputeStorage({ ownerField: undefined as any, key: { type: 'hash' }, storagePrefix: 'media' }) | ||
|
|
||
| // Clinics (owner = clinic id) | ||
| beforeChangeComputeStorage({ ownerField: 'clinic', key: { type: 'hash' }, storagePrefix: 'clinics' }) | ||
|
|
||
| // Users (owner = user id) | ||
| beforeChangeComputeStorage({ ownerField: 'user', key: { type: 'hash' }, storagePrefix: 'users' }) |
There was a problem hiding this comment.
The platform example is inconsistent with the implementation and tests. It should use a real (optional) owner field with ownerRequired: false and the 'platform' prefix; also avoid 'undefined as any'. Suggest updating to: beforeChangeComputeStorage({ ownerField: 'platformOwner', key: { type: 'hash' }, storagePrefix: 'platform', ownerRequired: false }).
Comment on lines
+51
to
+56
| Resulting path shapes: | ||
| - Platform content: `media/<hash>/<basename>` | ||
| - Clinic media: `clinics/<clinicId>/<hash>/<basename>` | ||
| - Clinic gallery media: `clinics/<clinicId>/gallery/<hash>/<basename>` | ||
| - Doctor media: `doctors/<doctorId>/<hash>/<basename>` | ||
| - User profile media: `users/<userId>/<hash>/<basename>` |
There was a problem hiding this comment.
The platform path should align with code/tests which use the 'platform' namespace, not 'media'. Update line 52 to: - Platform content: platform/<hash>/<basename>.
Comment on lines
+51
to
+56
| Resulting path shapes: | ||
| - Platform content: `media/<hash>/<basename>` | ||
| - Clinic media: `clinics/<clinicId>/<hash>/<basename>` | ||
| - Clinic gallery media: `clinics/<clinicId>/gallery/<hash>/<basename>` | ||
| - Doctor media: `doctors/<doctorId>/<hash>/<basename>` | ||
| - User profile media: `users/<userId>/<hash>/<basename>` |
There was a problem hiding this comment.
Docs state clinic gallery media includes a 'gallery/' segment, but the shared hook examples do not describe how that segment is introduced. Please either add a specific example showing how the collection inserts 'gallery/' (e.g., dedicated beforeChange for clinicGalleryMedia) or adjust the documented path to match the current computeStorage behavior.
Comment on lines
+78
to
+95
| // Always derive a hash-based folder key | ||
| const fileSize = extractFileSizeFromRequest(req) | ||
| const ownerSegment = owner ?? 'platform' | ||
| const filenameSegment = base ?? 'unknown' | ||
| const raw = `${ownerSegment}:${filenameSegment}${fileSize ? `:${fileSize}` : ''}` | ||
| folderKey = shortHash(raw) | ||
| keySource = 'hash' | ||
| } | ||
|
|
||
| // Ensure a stable key even when the configured source is missing. | ||
| if (!folderKey && operation === 'create') { | ||
| const fileSize = extractFileSizeFromRequest(req) | ||
| const ownerSegment = owner ?? 'platform' | ||
| const filenameSegment = base ?? 'unknown' | ||
| const raw = `${ownerSegment}:${filenameSegment}${fileSize ? `:${fileSize}` : ''}` | ||
| folderKey = shortHash(raw) | ||
| keySource = 'derived-hash' | ||
| } |
There was a problem hiding this comment.
The hash key derivation logic is duplicated. Consider extracting a small helper (e.g., deriveHashKey(owner, base, req)) to compute the raw string and shortHash once, then reuse it for both the 'hash' and 'derived-hash' branches.
| @@ -1,3 +1,4 @@ | |||
| import crypto from 'crypto' | |||
There was a problem hiding this comment.
Prefer importing from 'node:crypto' in ESM environments to avoid ambiguity and unintended polyfills: import crypto from 'node:crypto'. If updated, align the tests' vi.mock to mock 'node:crypto' accordingly.
Suggested change
| import crypto from 'crypto' | |
| import crypto from 'node:crypto' |
| expect(result.storagePath).toBe('platform/8686b7a110/hero.png') | ||
| }) | ||
|
|
||
| test('preserves existing storage path on update without filename', async () => { |
There was a problem hiding this comment.
The test name no longer matches the behavior (the hook now derives a hashed path on update using the original filename). Rename the test to reflect the new behavior, for example: 'derives hashed storage path on update without filename'.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request refactors media storage path logic across several collections to use a unified, predictable pattern via a shared
computeStoragehook. It also improves seed script reliability and consistency, updates documentation, and simplifies ownership enforcement for user profile media. The most important changes are grouped below.Media Storage Path Refactoring
computeStorageutility, ensuring consistent hashed folder keys and owner scoping. (src/collections/DoctorMedia/hooks/beforeChangeDoctorMedia.ts,src/collections/PlatformContentMedia/hooks/beforeChangePlatformContentMedia.ts,src/collections/UserProfileMedia/index.ts) [1] [2] [3] [4] [5] [6]beforeChangeUserProfileMediahook and replaced it with a declarative set of hooks, including ownership freezing and createdBy stamping, inUserProfileMedia. (src/collections/UserProfileMedia/hooks/beforeChangeUserProfileMedia.ts,src/collections/UserProfileMedia/index.ts) [1] [2] [3]Documentation Improvements
docs/integrations/storage.md)Seed Script Reliability and Consistency
src/endpoints/seed/clinics/clinics-seed.ts) [1] [2] [3]src/endpoints/seed/clinics/doctors-seed.ts) [1] [2]src/endpoints/seed/clinics/treatments-seed.ts) [1] [2] [3]Demo Seeding Improvements
src/endpoints/seed/demo/index.ts) [1] [2] [3]