Conversation
Promote the Fluid record access lint rule to error while keeping noUncheckedIndexedAccess disabled. Resolve reported record accesses without adding new production error paths. AB#34163 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
area: tools
area: driver
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (73 lines, 12 files), I've queued these reviewers:
How this works
|
Fleet Review — CleanNo issues found across the reviewer fleet for this run. |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified.
Pull request overview
Promotes unchecked record-access linting to an error for odsp-driver while preserving runtime behavior.
Changes:
- Refactors record access with safe narrowing and
Object.entries. - Documents format-guaranteed suppressions and assertions.
- Updates affected tests and ESLint configuration.
File summaries
| File | Description |
|---|---|
packages/drivers/odsp-driver/tsconfig.json |
Documents lint-based enforcement. |
packages/drivers/odsp-driver/src/test/prefetchSnapshotTests.spec.ts |
Adds snapshot-tree assertions. |
packages/drivers/odsp-driver/src/test/opsCaching.spec.ts |
Narrows cached batch data. |
packages/drivers/odsp-driver/src/test/getUrlAndHeadersWithAuth.spec.ts |
Asserts the authorization header exists. |
packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts |
Adds explicit tree and blob narrowing. |
packages/drivers/odsp-driver/src/odspSummaryUploadManager.ts |
Iterates records via Object.entries. |
packages/drivers/odsp-driver/src/odspSnapshotParser.ts |
Documents guaranteed parent lookup. |
packages/drivers/odsp-driver/src/odspDriverUrlResolver.ts |
Reuses narrowed header access. |
packages/drivers/odsp-driver/src/odspDocumentStorageServiceBase.ts |
Narrows optional protocol tree access. |
packages/drivers/odsp-driver/src/createFile/createNewUtils.ts |
Iterates summary records safely. |
packages/drivers/odsp-driver/src/compactSnapshotParser.ts |
Documents parser validation suppressions. |
packages/drivers/odsp-driver/eslint.config.mts |
Promotes the rule to error. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Bundle size comparisonBase commit: Pending — |
| "@typescript-eslint/strict-boolean-expressions": "off", | ||
| "unicorn/text-encoding-identifier-case": "off", | ||
| "@fluid-internal/fluid/no-unchecked-record-access": "warn", | ||
| "@fluid-internal/fluid/no-unchecked-record-access": "error", |
There was a problem hiding this comment.
I think we prefer to have this omitted as the default is error. Right?
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- IRequest headers are intentionally untyped. | ||
| const createNewHeader = request.headers?.[DriverHeader.createNew]; | ||
| if (createNewHeader) { |
There was a problem hiding this comment.
But we won't have an idea of the type if defined?
This looks like we expect type to be { fileName?: string }.
If so, recommend replacing this with use of a type guard helper to access this which will return undefined | { fileName?: string } to eliminate repeated unsafe defects.
| public async write(batchNumber: string, data: string): Promise<void> { | ||
| this.writeCount++; | ||
| this.data[batchNumber] = JSON.parse(data); | ||
| for (const op of this.data[batchNumber] as CacheEntry) { | ||
| const batch = JSON.parse(data) as CacheEntry; | ||
| this.data[batchNumber] = batch; | ||
| for (const op of batch) { |
There was a problem hiding this comment.
This code (typing) is so wrong. It really needs to be using JsonString and friends.
CacheEntry says array of IMessage | undefined but undefined is not allowed for serialized JSON.
Line 30 now 31 checks op for null, which is neither IMessage nor undefined
I almost don't want to touch this code because there are suspect things. But I don't see real harm here.
There was a problem hiding this comment.
I pushed a branch that tries to start straightening things out - I don't see how the code works. Must be missing something. See odsp-driver/bad-cache-handling-investigation - 9dc3e66
| const content = this.data[batchNumber]; | ||
| const content: unknown | undefined = this.data[batchNumber]; |
There was a problem hiding this comment.
This is useless. There appears to deficiency in the lint rule. unknown | undefined is just unknown.
Is the rule happy with : unknown?
There is no defect in this code as-is.
| // When we upload the container snapshot, we upload appTree in ".app" and protocol tree in ".protocol" | ||
| // So when we request the snapshot we get ".app" as tree and not as commit node as in the case just above. | ||
| // eslint-disable-next-line @fluid-internal/fluid/no-unchecked-record-access -- The snapshot format requires the .app tree; existing behavior fails if it is absent. | ||
| const hierarchicalAppTree = snapshotTree.trees[".app"]; |
There was a problem hiding this comment.
Sounds like the fix would be to add that requirement to the type.
Comment notes that it fails. I think that is suggesting that is a good thing but would be good to be more explicit. Say intentionally fails with ...
How contribute to this repo.
Guidelines for Pull Requests.
Description
Promotes
@fluid-internal/fluid/no-unchecked-record-accessfromwarntoerrorfor@fluidframework/odsp-driver, resolving AB#34163.The TypeScript
noUncheckedIndexedAccesssetting remains disabled because it is overly broad for array access. Record accesses are handled withObject.entries, existing assertions, explicit test narrowing, and targeted suppressions where ODSP format contracts guarantee the value. No new production error paths are introduced.Reviewer Guidance
The review process is outlined in the pull request guidelines.
Please focus on the record-access lint resolutions and documented suppressions for format-guaranteed values.