Enable easy strict flags in test tsconfig#1559
Conversation
Enable strictFunctionTypes, noImplicitThis, alwaysStrict, strictBindCallApply, and useUnknownInCatchVariables. Fix 19 errors.
Add non-null assertions to test files for: - EntityCoordinates.fromString() return values in setup functions - summary.described, summary.licensed, summary.files property access - Nested properties: sourceLocation, urls, attributions, natures
Fix ~240 null-safety issues across providers, business, routes, middleware, lib, and bin.
5f412bc to
cfd29b4
Compare
There was a problem hiding this comment.
Pull request overview
Enables several additional TypeScript strictness flags for the test build and updates tests and supporting runtime code to compile cleanly under the stricter settings.
Changes:
- Enabled
strictFunctionTypes,noImplicitThis,alwaysStrict,strictBindCallApply, anduseUnknownInCatchVariablesintest/tsconfig.json. - Updated test suites to satisfy stricter typing (notably
catcherror typing and nullable fields). - Tightened typings across providers/business code (type guards, return type adjustments, and non-null assertions) to resolve the resulting compilation errors.
Show a summary per file
| File | Description |
|---|---|
| test/tsconfig.json | Enables additional strict compiler flags for tests. |
| test/summary/scancode.ts | Adds non-null assertions for nullable summary fields in tests. |
| test/summary/reuse.ts | Casts catch error to Error for useUnknownInCatchVariables. |
| test/summary/licensee.ts | Casts catch error to Error for useUnknownInCatchVariables. |
| test/summary/fossology.ts | Adds non-null assertion to EntityCoordinates.fromString() result in tests. |
| test/summary/clearlyDefined.ts | Adds non-null assertions for nullable summary fields and coordinates in tests. |
| test/providers/summary/scancode/new-summarizer.mts | Casts catch error to Error. |
| test/providers/summary/scancode/legacy-summarizer.mts | Casts catch error to Error. |
| test/providers/store/mongoDefinition.ts | Casts catch error to Error. |
| test/providers/store/fileHarvest.ts | Casts catch error to Error. |
| test/providers/store/fileDefintion.ts | Casts catch error to Error. |
| test/providers/store/fileAttachment.ts | Casts catch error to Error. |
| test/providers/store/azblobDefinition.ts | Casts catch error to Error. |
| test/providers/store/azblobAttachment.ts | Casts catch error to Error. |
| test/providers/store/abstractFileStore.ts | Casts catch error to Error. |
| test/providers/caching/redis.ts | Casts catch error to Error. |
| test/lib/util.ts | Adjusts types/non-null assertions to satisfy stricter checks. |
| test/lib/spdx.ts | Adds casts for null test cases under stricter typing. |
| test/lib/licenseMatcher.ts | Adds non-null assertions for coordinates parsing in tests. |
| test/lib/curation.ts | Casts YAML-loaded value to match constructor input type. |
| test/lib/coordinatesMapper.ts | Adds non-null assertions for mapper results/coordinates parsing in tests. |
| routes/auth.ts | Adds type guards/non-null assertions for stricter nullability typing. |
| providers/upgrade/recomputeHandler.ts | Adds explicit cast on validate() return to satisfy stricter typing. |
| providers/upgrade/defVersionCheck.ts | Adds non-null assertions and adjusts getCoordinates signature to allow null. |
| providers/upgrade/azureQueueConfig.ts | Adds non-null assertions around config-derived connection strings. |
| providers/summary/scancode/new-summarizer.ts | Tightens null handling and normalizer return values under strict typing. |
| providers/summary/scancode/legacy-summarizer.ts | Tightens null handling and normalizer return values under strict typing. |
| providers/summary/scancode.ts | Adds non-null assertion when constructing summarizer via factory. |
| providers/summary/reuse.ts | Adjusts metadata/license normalization typing for strict mode. |
| providers/summary/licensee.ts | Replaces lodash get() usage with optional chaining and normalizes license typing. |
| providers/summary/fossology.ts | Non-null assertion for options assignment under strict mode. |
| providers/summary/clearlydefined.ts | Adds null/undefined-aware normalization and stricter provider indexing. |
| providers/stores/mongoConfig.ts | Adds explicit option typing/casts for strict mode. |
| providers/stores/mongo.ts | Aligns return types with DefinitionStore interface; adds non-null assertions around files. |
| providers/stores/fileHarvestStore.ts | Adds non-null assertions for URN parsing and tool/version indexing. |
| providers/stores/dispatchDefinitionStore.ts | Adjusts list/find typings with casts to satisfy strict checks. |
| providers/stores/azblobHarvestStore.ts | Adds non-null assertion for URN parsing. |
| providers/stores/azblobDefinitionStore.ts | Adds non-null assertion for coordinate parsing and adds find override to satisfy interface. |
| providers/stores/azblobConfig.ts | Adds explicit option typing/casts for strict mode. |
| providers/stores/abstractMongoDefinitionStore.ts | Adds non-null assertion for fromObject() result. |
| providers/stores/abstractFileStore.ts | Adds type-predicate filtering and non-null assertion for result coordinates parsing. |
| providers/search/memory.ts | Adds non-null assertion for EntityCoordinates.fromObject() result. |
| providers/search/azureConfig.ts | Adds non-null assertions around required config values. |
| providers/search/abstractSearch.ts | Ensures reductions handle missing arrays with ` |
| providers/queueing/memoryQueue.ts | Tightens queue message typing and non-null assertion on message text. |
| providers/queueing/azureStorageQueue.ts | Tightens queue message typing and fixes dequeueMultiple() return shaping. |
| providers/logging/winstonConfig.ts | Adds non-null assertions for AppInsights client usage under stricter typing. |
| providers/harvest/throttling/listBasedFilter.ts | Adds type predicates and non-null assertions for strict typing. |
| providers/harvest/crawlerQueueConfig.ts | Adds non-null assertions for config values and casts options for strict mode. |
| providers/harvest/crawlerConfig.ts | Adds non-null assertions for required config values and casts options for strict mode. |
| providers/harvest/azureQueueConfig.ts | Adds non-null assertions for required connection string. |
| providers/curation/mongoCurationStore.ts | Adds non-null assertions for curation data fields and coordinates conversions. |
| providers/curation/mongoConfig.ts | Adds non-null assertion for required connection string. |
| providers/curation/memoryStore.ts | Adds non-null assertions for curation data/coordinates. |
| providers/curation/githubConfig.ts | Tightens options typing and uses non-null assertion for required token. |
| providers/curation/github.ts | Adds numerous non-null assertions and typed filters/initializers under strict mode. |
| providers/curation/azureQueueConfig.ts | Adds non-null assertion for required connection string. |
| providers/caching/redisConfig.ts | Adds non-null assertions for required Redis config values. |
| middleware/githubConfig.ts | Adds non-null assertions for required GitHub token/team config entries. |
| middleware/github.ts | Adds non-null assertions for options/cache usage and annotates return types. |
| lib/licenseMatcher.ts | Handles potentially-null file lists and adds non-null assertions for coordinates/type/tool version. |
| business/suggestionService.ts | Uses a cast to force namespace = null for strict typing. |
| business/definitionService.ts | Adds non-null assertions and tighter filtering/coordinate parsing under strict mode. |
| bin/config.ts | Adds non-null assertions for rate-limit config parsing. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
providers/stores/dispatchDefinitionStore.ts:76
find()is declared to returnPromise<DefinitionFindResult>, but_performInSequence()can returnnull(e.g., if all stores fail/throw or a store returnsnull). Theas Promise<DefinitionFindResult>cast masks this and can cause runtime failures when callers expect{ data: ... }. Consider ensuringfind()always returns a validDefinitionFindResult(e.g.,{ data: [], continuationToken: undefined }) or throws when no underlying store can satisfy the operation.
find(query: DefinitionFindQuery, continuationToken = ''): Promise<DefinitionFindResult> {
return this._performInSequence(store => store.find(query, continuationToken)) as Promise<DefinitionFindResult>
}
- Files reviewed: 63/64 changed files
- Comments generated: 5
| if (!harvested?.reuse?.metadata!.CreatorTool) { | ||
| throw new Error('Invalid REUSE data') |
There was a problem hiding this comment.
harvested?.reuse?.metadata!.CreatorTool can throw a TypeError when reuse.metadata is missing, preventing the intended Invalid REUSE data error from being thrown. Use optional chaining for metadata (or explicitly check harvested.reuse?.metadata before reading CreatorTool) so invalid inputs fail gracefully with the expected error.
| list(coordinates: EntityCoordinates): Promise<string[]> { | ||
| return this._performInSequence(store => store.list(coordinates)) as Promise<string[]> | ||
| } |
There was a problem hiding this comment.
list() is typed to always return Promise<string[]>, but _performInSequence() can return null if all stores throw or return falsy results; the as Promise<string[]> cast hides that. This can lead to runtime errors when callers assume an array. Consider making _performInSequence accept a required default value (e.g., [] for list) or have list() explicitly fall back to an empty array (or throw) instead of returning null.
This issue also appears on line 74 of the same file.
| // @ts-expect-error - AzBlob store does not support find; satisfies DefinitionStore interface | ||
| override async find(_query: DefinitionFindQuery, _continuationToken?: string): Promise<DefinitionFindResult> { | ||
| return super.find() as unknown as DefinitionFindResult |
There was a problem hiding this comment.
This find() override returns super.find() which is implemented to return null in AbstractAzBlobStore, but the method is typed as returning DefinitionFindResult. Returning null here violates the DefinitionStore contract and can break code paths if this store is used without a dispatch fallback. Prefer explicitly throwing an "unsupported" error or returning an empty DefinitionFindResult ({ data: [] }) to satisfy the interface without relying on casts.
| // @ts-expect-error - AzBlob store does not support find; satisfies DefinitionStore interface | |
| override async find(_query: DefinitionFindQuery, _continuationToken?: string): Promise<DefinitionFindResult> { | |
| return super.find() as unknown as DefinitionFindResult | |
| /** | |
| * AzBlob store does not support find; return an empty result to satisfy | |
| * the DefinitionStore contract without returning null. | |
| */ | |
| override async find(_query: DefinitionFindQuery, _continuationToken?: string): Promise<DefinitionFindResult> { | |
| return { data: [] } |
| async validate(definition: Definition | null): Promise<Definition | null> { | ||
| return this._upgradePolicy.validate(definition) | ||
| return this._upgradePolicy.validate(definition) as Promise<Definition | null> | ||
| } |
There was a problem hiding this comment.
validate() is declared as Promise<Definition | null> but the underlying UpgradeHandler.validate contract (and implementations like DefinitionVersionChecker.validate) can return undefined to indicate "needs recompute". Casting away undefined (as Promise<Definition | null>) can mislead callers/types and still allows undefined to flow at runtime. Consider keeping the return type as Promise<Definition | null | undefined> and returning the policy result without a cast.
| async _getRelatedDefinitions(coordinates: EntityCoordinates): Promise<RelatedDefinitions | null> { | ||
| const query = coordinates.asRevisionless() | ||
| query.namespace = query.namespace ? query.namespace : null // explicitly exclude namespace | ||
| query.namespace = query.namespace ? query.namespace : (null as unknown as string) // explicitly exclude namespace | ||
| const results = await this.definitionService.find(query) |
There was a problem hiding this comment.
query.namespace is being assigned null via (null as unknown as string), which defeats type checking and makes it hard to reason about the query shape. Since downstream query logic appears to intentionally treat namespace: null as "explicitly match no-namespace definitions", consider updating the query type to allow namespace?: string | null (e.g., DefinitionFindQuery.namespace) or introducing a dedicated query type for this use case, rather than casting null to string.
Enables strictFunctionTypes, noImplicitThis, alwaysStrict, strictBindCallApply, and useUnknownInCatchVariables in test/tsconfig.json. Fixes the 19 resulting errors.