fix: make nullable entity fields explicitly typed#380
fix: make nullable entity fields explicitly typed#380silent-cipher wants to merge 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #205 by aligning TypeORM entity and related DTO/service code with the database reality that nullable columns can be null, reducing unsafe assumptions and runtime errors across metrics, jobs, deal flow, and IPNI logic.
Changes:
- Update multiple TypeORM entities to type
nullablecolumns asT | null(instead of non-null/optional). - Add/adjust null handling in metrics/services, deal flow metrics computations, and IPNI verification/monitoring.
- Update selected DTOs to reflect nullable fields (e.g., provider/service URL).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/backend/src/metrics/services/providers.service.ts | Avoids crashing when serviceTypes is null by using optional chaining. |
| apps/backend/src/metrics/services/failed-deals.service.ts | Maps nullable dataset IDs to undefined for optional DTO fields. |
| apps/backend/src/metrics/dto/provider-performance.dto.ts | Marks serviceUrl as nullable in DTO and Swagger schema. |
| apps/backend/src/metrics/dto/failed-retrievals.dto.ts | Adjusts providerId typing to reflect DB nullability. |
| apps/backend/src/jobs/jobs.service.ts | Normalizes nullable providerId to undefined where callers expect optional. |
| apps/backend/src/ipni/ipni-verification.service.ts | Adds guard for missing service URL and tightens expected-provider construction input type. |
| apps/backend/src/dev-tools/dev-tools.service.ts | Normalizes nullable deal fields in dev-tools responses and mappings. |
| apps/backend/src/deal/deal.service.ts | Makes ingest/on-chain metric fields nullable; improves ingest latency/throughput computation guards and logging. |
| apps/backend/src/deal-addons/strategies/ipni.strategy.ts | Adds explicit runtime failures for missing service URL / piece CID; switches entity import to type-only. |
| apps/backend/src/database/entities/storage-provider.entity.ts | Types nullable providerId/serviceUrl as `T |
| apps/backend/src/database/entities/retrieval.entity.ts | Types nullable retrieval columns as `T |
| apps/backend/src/database/entities/job-schedule-state.entity.ts | Types lastRunAt as `Date |
| apps/backend/src/database/entities/deal.entity.ts | Types many nullable deal columns as `T |
| apps/backend/src/common/logging.ts | Allows pieceCid to be `string |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SgtPooki
left a comment
SgtPooki
left a comment
There was a problem hiding this comment.
there are a lot of merge conflicts, and this PR has a lot of changes that i haven't got to look into deeply..
I would much rather see a smaller change since we're messing with metrics we're exporting
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I haven't got a chance to look at this deeply
| dealLatencyMs: deal.dealLatencyMs ?? undefined, | ||
| dealLatencyWithIpniMs: deal.dealLatencyWithIpniMs ?? undefined, | ||
| ingestLatencyMs: deal.ingestLatencyMs ?? undefined, | ||
| ipniTimeToIndexMs: deal.ipniTimeToIndexMs ?? undefined, | ||
| ipniTimeToAdvertiseMs: deal.ipniTimeToAdvertiseMs ?? undefined, | ||
| ipniTimeToVerifyMs: deal.ipniTimeToVerifyMs ?? undefined, | ||
| serviceTypes: deal.serviceTypes ?? [], | ||
| spAddress: deal.spAddress, | ||
| errorMessage: deal.errorMessage, | ||
| errorMessage: deal.errorMessage ?? undefined, |
There was a problem hiding this comment.
i dont think the goal of this is to make things possible to be undefined.. we want them to be defined to have strict type awareness as much as possible, right?
if these were set to values before, adding a fallback to undefined is a regression isn't it?
github-project-automation
bot
moved this from 🔎 Awaiting review
to ⌨️ In Progress
in FOC
closes #205