[Gel] Add Product queries | refactor service/repo layers
#3332
Conversation
📝 WalkthroughWalkthroughRefactors product types and repository/service flows, introduces an EdgeDB-backed repository, updates DTO field types, adjusts a migration to new APIs/data shapes, tweaks resolver access for nested IDs, and consolidates create/read/update/delete paths to return secured DTOs via repository-based methods. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
bryanjnelson
changed the title
Product queries | refactor service/repo layers appropriatelyProduct queries | refactor service/repo layers appropriately
bryanjnelson
force-pushed
the
0938-edgedb-product-queries
branch
3 times, most recently
from
5aa5778 to
1b1a3fc
Compare
bryanjnelson
changed the title
Product queries | refactor service/repo layers appropriatelyProduct queries | refactor service/repo layers appropriately
bryanjnelson
changed the title
Product queries | refactor service/repo layers appropriatelyProduct queries | refactor service/repo layers
bryanjnelson
force-pushed
the
0938-edgedb-product-queries
branch
2 times, most recently
from
9f9fe90 to
b7567c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/product/product.service.ts (1)
88-105: Derivative totals computed as 0 when no override.
IfscriptureReferencesOverrideis undefined, totals should fall back to the producible’s scripture references.- totalVerses = getTotalVerses( - ...(derivativeInput.scriptureReferencesOverride ?? []), - ); - totalVerseEquivalents = getTotalVerseEquivalents( - ...(derivativeInput.scriptureReferencesOverride ?? []), - ); + const effectiveScripture = + derivativeInput.scriptureReferencesOverride ?? + (await this.repo.getProducibleScriptureReferences( + derivativeInput.produces, + )); + totalVerses = getTotalVerses(...effectiveScripture); + totalVerseEquivalents = getTotalVerseEquivalents(...effectiveScripture);If
getProducibleScriptureReferences()doesn’t exist, I can add it to both repos.
🧹 Nitpick comments (6)
src/components/product/dto/product.dto.ts (1)
269-274: Exporting ProductConcretes: addas constfor stronger typing.
Prevents key widening and helps downstreamkeyofusages.-export const ProductConcretes = { +export const ProductConcretes = { DirectScriptureProduct, DerivativeScriptureProduct, OtherProduct, -}; +} as const;src/components/product/product.edgedb.repository.ts (1)
181-202: Return types areany; tighten them for safety.
Helps callers and prevents accidental shape drift.-import { type ID, type PublicOf } from '../../common'; +import { type ID, type PublicOf, type UnsecuredDto } from '../../common'; @@ - Product, + Product, + DirectScriptureProduct, + DerivativeScriptureProduct, + OtherProduct, } from './dto'; @@ - async createDerivative( + async createDerivative( input: CreateDerivativeScriptureProduct & { totalVerses: number; totalVerseEquivalents: number; }, - ) { + ): Promise<UnsecuredDto<DerivativeScriptureProduct>> { return await this.concretes.DerivativeScriptureProduct.create(input); } @@ - async createDirect( + async createDirect( input: CreateDirectScriptureProduct & { totalVerses: number; totalVerseEquivalents: number; }, - ) { + ): Promise<UnsecuredDto<DirectScriptureProduct>> { return await this.concretes.DirectScriptureProduct.create(input); } @@ - async createOther(input: CreateOtherProduct) { + async createOther(input: CreateOtherProduct): Promise<UnsecuredDto<OtherProduct>> { return await this.concretes.OtherProduct.create(input); }src/components/product/product.service.ts (2)
122-147: Create permission is verified after write.
This can write unauthorized records if repo doesn’t enforce auth. Prefer verifying before persisting.- const created = otherInput + // Verify create permission before persisting; use a synthetic DTO if needed. + // const draftDto = this.buildDraftDto(input, steps, progressTarget, totalVerses, totalVerseEquivalents); + // this.privileges.for(resolveProductType(draftDto), draftDto).verifyCan('create'); + + const created = otherInput ? await this.repo.createOther({ ...otherInput, progressTarget, steps }) : 'produces' in input ? await this.repo.createDerivative({ ...input, progressTarget, steps, totalVerses, totalVerseEquivalents, }) : await this.repo.createDirect({ ...input, progressTarget, steps, totalVerses, totalVerseEquivalents, }); const securedCreated = this.secure(created); - this.privileges + this.privileges .for(resolveProductType(securedCreated), securedCreated) .verifyCan('create'); return securedCreated;
173-191: UpdateDirect flow looks sound; keep TODO in mind.
Consider moving description-merge into repo later.src/components/product/product.repository.ts (2)
99-116: Clarify return types for readOne/readOneUnsecured.readOne returns a hydrated DB row (dto), while readOneUnsecured returns an UnsecuredDto. Add explicit return types to prevent misuse at call sites.
Apply:
- async readOne(id: ID) { + async readOne(id: ID): Promise<HydratedProductRow> { - async readOneUnsecured(id: ID) { + async readOneUnsecured(id: ID): Promise<UnsecuredDto<AnyProduct>> {
331-350: Annotate creation method return types for stronger guarantees.These methods return Unsecured DTOs; annotate them (including the private create path).
Apply:
- async createDerivative( + async createDerivative( input: CreateDerivativeScriptureProduct & { totalVerses: number; totalVerseEquivalents: number; }, - ) { + ): Promise<UnsecuredDto<DerivativeScriptureProduct>> { return (await this.create( input, )) as UnsecuredDto<DerivativeScriptureProduct>; } - async createDirect( + async createDirect( input: CreateDirectScriptureProduct & { totalVerses: number; totalVerseEquivalents: number; }, - ) { + ): Promise<UnsecuredDto<DirectScriptureProduct>> { return (await this.create(input)) as UnsecuredDto<DirectScriptureProduct>; } - private async create( + private async create( input: (CreateDerivativeScriptureProduct | CreateDirectScriptureProduct) & { totalVerses: number; totalVerseEquivalents: number; }, - ) { + ): Promise<UnsecuredDto<AnyProduct>> {Also applies to: 351-356
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/components/product/dto/product.dto.ts(3 hunks)src/components/product/migrations/fix-nan-total-verse-equivalents.migration.ts(1 hunks)src/components/product/product.edgedb.repository.ts(1 hunks)src/components/product/product.repository.ts(14 hunks)src/components/product/product.resolver.ts(1 hunks)src/components/product/product.service.ts(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/components/product/dto/product.dto.ts (1)
src/core/resources/resource.loader.ts (1)
LinkTo(34-39)
src/components/product/product.edgedb.repository.ts (3)
src/common/types.ts (1)
PublicOf(28-28)src/common/instance-maps.ts (1)
grabInstances(10-15)src/common/id-field.ts (1)
ID(24-25)
src/components/product/migrations/fix-nan-total-verse-equivalents.migration.ts (1)
src/components/scripture/verse-equivalents.ts (1)
getTotalVerseEquivalents(12-22)
src/components/product/product.repository.ts (6)
src/components/scripture/scripture-reference.service.ts (1)
ScriptureReferenceService(10-69)src/common/secured-property.ts (1)
UnsecuredDto(57-59)src/components/product/dto/product.dto.ts (1)
AnyProduct(229-232)src/core/database/results/parse-base-node.ts (1)
BaseNode(5-12)src/common/exceptions/exception.ts (1)
ServerException(14-14)src/components/product/dto/producible.dto.ts (2)
ProducibleType(49-49)ProducibleType(50-55)
🔇 Additional comments (24)
src/components/product/dto/product.dto.ts (2)
21-21: Good: type-only import for LinkTo avoids runtime side effects.
58-60: Switch to LinkTo shape is fine; ensure resolvers use.id.
Project resolver’s OnlyId branch still returns{ id: product.project }. It must useproduct.project.id. See resolver comment with fix.src/components/product/product.edgedb.repository.ts (4)
18-41: Potential invalid.slice(9, null)usage in GEL expression.
Confirm thatslice(9, null)is valid; most APIs treatundefined/omission as “to end”.- labels: e.array_agg(e.set(product.engagement.__type__.name.slice(9, null))), + labels: e.array_agg(e.set(product.engagement.__type__.name.slice(9))),
77-91: Verifyproduces.scriptureReferencesshape matches GraphQL DTO.
e.tuple([dsp.produces.scripture])might not equal the expectedScriptureRange[]. Consider returning the set directly orarray_agg.- produces: { - scriptureReferences: e.tuple([dsp.produces.scripture]), + produces: { + // If multiple ranges, aggregate to array; adjust to match SecuredProducible mapping + scriptureReferences: e.array_agg(dsp.produces.scripture), createdAt: dsp.produces.createdAt, id: dsp.produces.id, },
106-164: Concrete repo providers: ensure they’re registered in the module.
These classes must be included inproviderssoModuleRef.get()can resolve them.
165-172: Repo base looks good.
Hydrate/omit config aligns with DTOs.src/components/product/migrations/fix-nan-total-verse-equivalents.migration.ts (2)
30-32: Good: pass inner.valueto VEs calculation.
Matches expected API ofgetTotalVerseEquivalents.
27-27: Prefer direct repository reads in migrations: secured service reads (e.g.productService.readMany) can omitscriptureReferences.value. Use the repository’s unsecured method to load all fields:- const products = await this.productService.readMany(ids); + // Unsecured read to include scriptureReferences + const products = await this.productService['repo'].readManyUnsecured(ids);Or inject the repository into the migration instead of relying on the secured DTO.
src/components/product/product.service.ts (6)
62-63: Good: validate engagement existence up front.
Avoids orphaned products.
155-162: Good: readOne now returns secured DTOs.
Aligns service API and callers.
251-259: UpdateDerivative mirrors direct; OK.
311-340: UpdateOther: reads unsecured, verifies, merges description, then updates — OK.
423-428: Delete path: read unsecured then enforce privilege — OK.
432-437: List path: secure items after repo list — OK.src/components/product/product.repository.ts (10)
12-12: Imports and type wiring look consistent.New dependencies and DTO types align with usages below.
Also applies to: 17-21, 23-23, 39-44, 52-52, 68-68, 70-70
91-96: Confirm DI provider registration and avoid circulars.Ensure ScriptureReferenceRepository, ScriptureReferenceService, and ResourceResolver are provided in the module and don’t introduce circular deps. If any are optional, mark accordingly.
127-133: LGTM: Unsecured mapper over readMany.Mapping hydrated rows to UnsecuredDto keeps the API consistent with the new DTO flow.
276-278: Breaking shape change: engagement/project now nested objects.Downstream DTOs/resolvers expecting raw IDs must be updated to handle
{ id }. Verify GraphQL mappers and any serialization that previously used flat IDs.
288-305: LGTM: Direct updates split scripture + unspecifiedScripture from simple props.Separation minimizes unintended property writes and uses ScriptureReferenceService correctly.
443-444: LGTM: Create flows return mapped DTOs.Returning mapped DTOs immediately after creation simplifies caller logic and keeps shapes uniform.
Also applies to: 480-483
577-587: LGTM: Other updates return mapped DTO.Consistent with other update paths; minimal surface area for callers.
619-623: LGTM: Paginated list maps items to DTOs.Keeps pagination metadata intact while normalizing item shape.
648-657: Good error wrapping on delete.ServerException with cause retains context while hiding low-level details.
724-747: LGTM: Derivative mapping with override semantics.Resolve producible type from labels and switch scripture refs based on isOverriding—clean and readable.
| async listIdsWithPnpIndexes(engagementId: ID, _type?: string) { | ||
| const engagement = e.cast(e.LanguageEngagement, e.uuid(engagementId)); | ||
|
|
||
| const query = e.select(e.Product, (p) => ({ | ||
| id: true, | ||
| pnpIndex: p.pnpIndex, | ||
| ...e.is(e.DirectScriptureProduct, {}), | ||
| filter: e.op(p.engagement, '=', engagement), | ||
| })); | ||
|
|
||
| return await this.db.run(query); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type filter is ignored in listIdsWithPnpIndexes.
Implement the filter or drop the param.
- async listIdsWithPnpIndexes(engagementId: ID, _type?: string) {
+ async listIdsWithPnpIndexes(engagementId: ID, _type?: string) {
const engagement = e.cast(e.LanguageEngagement, e.uuid(engagementId));
-
- const query = e.select(e.Product, (p) => ({
- id: true,
- pnpIndex: p.pnpIndex,
- ...e.is(e.DirectScriptureProduct, {}),
- filter: e.op(p.engagement, '=', engagement),
- }));
+ const query =
+ _type === 'DirectScriptureProduct'
+ ? e.select(e.DirectScriptureProduct, (p) => ({
+ id: true,
+ pnpIndex: p.pnpIndex,
+ filter: e.op(p.engagement, '=', engagement),
+ }))
+ : e.select(e.Product, (p) => ({
+ id: true,
+ pnpIndex: p.pnpIndex,
+ filter: e.op(p.engagement, '=', engagement),
+ }));
return await this.db.run(query);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async listIdsWithPnpIndexes(engagementId: ID, _type?: string) { | |
| const engagement = e.cast(e.LanguageEngagement, e.uuid(engagementId)); | |
| const query = e.select(e.Product, (p) => ({ | |
| id: true, | |
| pnpIndex: p.pnpIndex, | |
| ...e.is(e.DirectScriptureProduct, {}), | |
| filter: e.op(p.engagement, '=', engagement), | |
| })); | |
| return await this.db.run(query); | |
| } | |
| } | |
| async listIdsWithPnpIndexes(engagementId: ID, _type?: string) { | |
| const engagement = e.cast(e.LanguageEngagement, e.uuid(engagementId)); | |
| const query = | |
| _type === 'DirectScriptureProduct' | |
| ? e.select(e.DirectScriptureProduct, (p) => ({ | |
| id: true, | |
| pnpIndex: p.pnpIndex, | |
| filter: e.op(p.engagement, '=', engagement), | |
| })) | |
| : e.select(e.Product, (p) => ({ | |
| id: true, | |
| pnpIndex: p.pnpIndex, | |
| filter: e.op(p.engagement, '=', engagement), | |
| })); | |
| return await this.db.run(query); | |
| } | |
| } |
bryanjnelson
force-pushed
the
0938-edgedb-product-queries
branch
from
b7567c7 to
c107d85
Compare
bryanjnelson
force-pushed
the
0938-edgedb-product-queries
branch
from
c107d85 to
a0457a4
Compare
Partially addresses #3454.
Where this stands:
- the Gel queries are blocked due to an issue where the TS server crashes on account of the types being created- this issue happened before and Carson was able to modify the types to eliminate the complexity causing it- after that is resolved then further work can be done to complete the Gel queries