Fix #4651 default asset languagecode#4662
Conversation
|
Vendure Core — View preview |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA 2 out of 3 committers have signed the CLA. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded RequestContextService.createDefaultContext(config?) which builds a RequestContext from the default Channel (deriving languageCode and currencyCode) and fixed defaults for apiType/isAuthorized/authorizedAsOwnerOnly. Refactored RequestContextService.create to accept an exported CreateRequestContextConfig interface and added a CreateDefaultRequestContextConfig type alias. AssetService now injects RequestContextService and calls createDefaultContext() when no RequestContext is provided. Extended allowed asset file types to include 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I created the e2e test first, confirmed it failed and then added the fix. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/core/src/service/helpers/request-context/request-context.service.ts (1)
41-51: LGTM — sensible fallback chain forlanguageCode.The
channel.defaultLanguageCode ?? this.configService.defaultLanguageCodefallback is a nice defensive touch: althoughChannel.defaultLanguageCodeis a non-nullable column, the extra guard handles the edge case where the default channel is somehow missing the value, matching the intent of restoring pre-v3.6 behaviour.One minor thought:
isAuthorized: true/authorizedAsOwnerOnly: falsemirrorsRequestContext.empty(), so callers effectively get full admin authority. That's consistent with existing behaviour, but worth being explicit about in the JSDoc so users know this context bypasses permission checks — same caveat that applies toRequestContext.empty().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/helpers/request-context/request-context.service.ts` around lines 41 - 51, Add a JSDoc comment to createDefaultContext explaining that it sets isAuthorized: true and authorizedAsOwnerOnly: false (matching RequestContext.empty()) and therefore grants full admin authority and bypasses permission checks; mention that callers should treat this context as an elevated/admin context and that the languageCode fallback uses channel.defaultLanguageCode ?? this.configService.defaultLanguageCode to preserve pre-v3.6 behavior.packages/core/e2e/asset-create-from-file-stream.e2e-spec.ts (2)
19-25: Consider skipping the products CSV import for this focused test.
server.initwithproductsCsvPathandcustomerCount: 1pulls in unrelated fixtures just to exercisecreateFromFileStream. Since the test only needs a running server + default channel, you can speed this suite up and reduce coupling to the fixtures directory by dropping the CSV / customer seed:Proposed simplification
- beforeAll(async () => { - await server.init({ - initialData, - productsCsvPath: path.join(__dirname, 'fixtures/e2e-products-full.csv'), - customerCount: 1, - }); - }, TEST_SETUP_TIMEOUT_MS); + beforeAll(async () => { + await server.init({ + initialData, + customerCount: 0, + }); + }, TEST_SETUP_TIMEOUT_MS);If you drop the CSV, the
pathimport can also go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/e2e/asset-create-from-file-stream.e2e-spec.ts` around lines 19 - 25, The test seeds unrelated fixtures by passing productsCsvPath and customerCount to server.init; remove those options from the server.init call in asset-create-from-file-stream.e2e-spec.ts so the test only starts the server with initialData (and the default channel), and also remove the unused path import at the top of the file. Update the beforeAll to call server.init({ initialData }) (leave TEST_SETUP_TIMEOUT_MS unchanged) and delete the productsCsvPath and customerCount references so the suite runs faster and is decoupled from fixtures.
39-45: Tighten the assertion — the'id' in resultguard can hide a regression.If
createFromFileStreamever returns aMimeTypeError(which also has noid), the current assertions would fail viaexpect('id' in result).toBe(true)— good — but theresult.namecheck is then silently skipped by theif ('id' in result)guard, so any future regression in the default translation name wouldn't fail the test.Consider asserting the error-free path more directly so the name check always executes:
Proposed tweak
- const result = await assetService.createFromFileStream(stream, 'test-file.txt'); - - expect(result).toBeDefined(); - expect('id' in result).toBe(true); - if ('id' in result) { - expect(result.name).toBe('test-file.txt'); - } + const result = await assetService.createFromFileStream(stream, 'test-file.txt'); + + expect(result).toBeDefined(); + if (!('id' in result)) { + throw new Error(`Expected Asset, got error result: ${JSON.stringify(result)}`); + } + expect(result.name).toBe('test-file.txt'); + expect(result.languageCode).toBeDefined();The extra
languageCodeassertion would also explicitly cover the issue#4651regression you're guarding against.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/e2e/asset-create-from-file-stream.e2e-spec.ts` around lines 39 - 45, The test currently uses an `if ('id' in result)` guard which can silently skip the name assertion if `result` is an error-like object; change the assertions to explicitly require the success shape so failures aren’t masked: assert that `result.id` is defined (e.g., expect(result.id).toBeDefined()), then unconditionally assert `result.name` equals 'test-file.txt' and add an explicit assertion for `result.languageCode` (e.g., expect(result.languageCode).toBe('en')) after calling assetService.createFromFileStream so the success path is enforced and the `#4651` regression is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/e2e/asset-create-from-file-stream.e2e-spec.ts`:
- Around line 19-25: The test seeds unrelated fixtures by passing
productsCsvPath and customerCount to server.init; remove those options from the
server.init call in asset-create-from-file-stream.e2e-spec.ts so the test only
starts the server with initialData (and the default channel), and also remove
the unused path import at the top of the file. Update the beforeAll to call
server.init({ initialData }) (leave TEST_SETUP_TIMEOUT_MS unchanged) and delete
the productsCsvPath and customerCount references so the suite runs faster and is
decoupled from fixtures.
- Around line 39-45: The test currently uses an `if ('id' in result)` guard
which can silently skip the name assertion if `result` is an error-like object;
change the assertions to explicitly require the success shape so failures aren’t
masked: assert that `result.id` is defined (e.g.,
expect(result.id).toBeDefined()), then unconditionally assert `result.name`
equals 'test-file.txt' and add an explicit assertion for `result.languageCode`
(e.g., expect(result.languageCode).toBe('en')) after calling
assetService.createFromFileStream so the success path is enforced and the `#4651`
regression is covered.
In
`@packages/core/src/service/helpers/request-context/request-context.service.ts`:
- Around line 41-51: Add a JSDoc comment to createDefaultContext explaining that
it sets isAuthorized: true and authorizedAsOwnerOnly: false (matching
RequestContext.empty()) and therefore grants full admin authority and bypasses
permission checks; mention that callers should treat this context as an
elevated/admin context and that the languageCode fallback uses
channel.defaultLanguageCode ?? this.configService.defaultLanguageCode to
preserve pre-v3.6 behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1cacf2a-a6cb-4618-85d6-fa6b5118369e
📒 Files selected for processing (4)
docs/docs/reference/typescript-api/request/request-context-service.mdxpackages/core/e2e/asset-create-from-file-stream.e2e-spec.tspackages/core/src/service/helpers/request-context/request-context.service.tspackages/core/src/service/services/asset.service.ts
|
I have read the CLA Document and I hereby sign the CLA |
…o asset.e2e-spec.ts
…iguration parameters
…code' into fix-vendurehq#4651-default-asset-languagecode
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/e2e/asset.e2e-spec.ts`:
- Around line 363-378: The test "create an asset from a file stream without
RequestContext" is using an unsupported extension ('test-file.txt') and either
should assert a MIME-type error or use a permitted extension and clean up the
created asset; update the call to assetService.createFromFileStream to use a
permitted filename (e.g. 'test-file.png') and after a successful create call
assetService.delete(result.id) (or the appropriate deletion method on
AssetService) to remove the created asset so the shared assets.totalItems
assertion is not affected; alternatively change the expectation to assert that
createFromFileStream throws a MIME type error when given 'test-file.txt'
(referencing AssetService.createFromFileStream and the suite's assets.totalItems
assertion).
In
`@packages/core/src/service/helpers/request-context/request-context.service.ts`:
- Around line 42-44: The createDefaultContext method's config parameter is
currently required but should be optional per the JSDoc; make it optional by
giving the parameter a default of an empty object so callers can invoke
createDefaultContext() with no args. Update the signature of
createDefaultContext(config: Partial<ConstructorParameters<typeof
RequestContext>[0]> = {}) to provide the {} default, ensuring RequestContext
construction inside the method still works with partial/empty config (adjust any
destructuring or usage inside createDefaultContext to handle missing properties
if necessary).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b68d273-45ab-4203-8037-425bd16a62f2
📒 Files selected for processing (3)
docs/docs/reference/typescript-api/request/request-context-service.mdxpackages/core/e2e/asset.e2e-spec.tspackages/core/src/service/helpers/request-context/request-context.service.ts
✅ Files skipped from review due to trivial changes (1)
- docs/docs/reference/typescript-api/request/request-context-service.mdx
2 participants
Description
Fixes #4651.
Since v3.6,
AssetService.createFromFileStream()fails with a NOT NULL constraint violation onasset_translation.languageCodewhen called without an explicitRequestContext. The root cause: the fallback usedRequestContext.empty(), which constructsnew Channel()with nodefaultLanguageCode, leavingctx.languageCodeasundefinedwhencreateAssetInternalbuilds the default translation.This PR introduces a new
RequestContextService.createDefaultContext()method that builds aRequestContextbased on the defaultChannel, guaranteeing a validlanguageCode(falling back toConfigService.defaultLanguageCodeif the channel has none) andcurrencyCode.AssetService.createFromFileStream()now uses this helper instead of RequestContext.empty()` when no ctx is supplied, restoring the pre-v3.6 behavior.Changes:
packages/core/src/service/helpers/request-context/request-context.service.ts— addcreateDefaultContext()(admin API type, authorized, derived from the default channel).packages/core/src/service/services/asset.service.ts— injectRequestContextServiceand usecreateDefaultContext()as the fallback increateFromFileStream.packages/core/e2e/asset-create-from-file-stream.e2e-spec.ts— new e2e test reproducing the original issue by callingcreateFromFileStreamwithout aRequestContext.docs/.../request-context-service.mdx— documents the new method.Breaking changes
None.
createDefaultContext()is additive, and the asset service change only affects the previously-broken path where noRequestContextis passed (which threw a DB constraint error before).Screenshots
N/A — backend-only change.
Checklist
📌 Always:
👍 Most of the time: