ci(qodo): add optional chaining compliance rule by pkuczynski · Pull Request #12255 · typeorm/typeorm

pkuczynski · 2026-03-22T22:24:20Z

Add a Qodo compliance check for preferring optional chaining (?.) over non-null assertions (!) in tests after null checks.

SonarQube frequently flags non-null assertions as unnecessary type assertions (typescript:S4325). Adding this to the Qodo checklist catches it earlier during PR review, before it reaches the SonarQube quality gate.

const result = await repository.findOne({ where: { id: 1 } })

// Before (non-null assertion)
expect(result!.name).to.equal("foo")

// After (optional chaining)
expect(result).to.not.be.null
expect(result?.name).to.equal("foo")

qodo-free-for-open-source-projects · 2026-03-22T22:24:31Z

Review Summary by Qodo

Add optional chaining compliance rule to Qodo checklist

✨ Enhancement

Walkthroughs

Description

• Add optional chaining compliance rule to Qodo checklist
• Catches non-null assertion issues before SonarQube quality gate
• Aligns with TypeScript best practices for null safety

Diagram

flowchart LR
  A["PR Compliance Checklist"] -->|adds new rule| B["Optional Chaining Rule"]
  B -->|objective| C["Use ?. instead of !"]
  B -->|success| D["expect().to.not.be.null + ?.property"]
  B -->|failure| E["result!.property usage"]

File Changes

1. pr_compliance_checklist.yaml ✨ Enhancement +6/-0

Add optional chaining compliance rule

• Added new compliance rule for optional chaining preference in tests
• Rule specifies using optional chaining (?.) instead of non-null assertions (!)
• Includes objective, success criteria, and failure criteria for the rule
• Positioned between "Consistent Naming Conventions" and "Remove AI-generated noise" rules

pr_compliance_checklist.yaml

qodo-free-for-open-source-projects · 2026-03-22T22:28:07Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. ~~Skipped loop assertions~~ ☑ 🐞 Bug ✓ Correctness

Description

The success criteria recommends result?.items ?? [] for iteration, which can cause loop bodies
(and any assertions inside) to be skipped when result is null, allowing tests to pass without
exercising checks.

Code

pr_compliance_checklist.yaml[R23-24]

+      success_criteria: "Test assertions use expect(result).to.not.be.null followed by result?.property and result?.items ?? [] for iteration."
+      failure_criteria: "Tests use result!.property or result!.items non-null assertions after null checks."

Evidence

The checklist text explicitly endorses result?.items ?? [] as a preferred iteration pattern. In
JS/TS, (null as any)?.items evaluates to undefined, and undefined ?? [] becomes [], so `for
(... of [])` performs zero iterations and silently skips any assertions that were supposed to run.

pr_compliance_checklist.yaml[20-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The compliance rule’s success criteria recommends iterating with `result?.items ?? []`, which can silently skip the loop body when `result` is `null`, allowing tests to pass without executing assertions.
### Issue Context
This is a checklist rule, so its wording will influence how reviewers/automation judge PRs. The recommended iteration pattern weakens tests by turning missing data into an empty collection.
### Fix Focus Areas
- pr_compliance_checklist.yaml[20-24]
### Suggested change
Update the success/failure criteria to avoid recommending `?? []` for iteration after a null-check expectation. Prefer either:
- using a real guard (e.g., assert non-null then use `result.items`), or
- explicitly failing when `items` is missing, rather than defaulting to `[]`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Non-null assertion overreach~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The objective/failure criteria reads like a blanket replacement of ! with ?. after null checks,
but optional chaining is not a semantic replacement for non-null assertions used for mutation or for
passing values to APIs, making the rule impractical and likely to cause noisy enforcement.

Code

pr_compliance_checklist.yaml[R22-24]

+      objective: "Tests should use optional chaining (?.) instead of non-null assertions (!) after null checks."
+      success_criteria: "Test assertions use expect(result).to.not.be.null followed by result?.property and result?.items ?? [] for iteration."
+      failure_criteria: "Tests use result!.property or result!.items non-null assertions after null checks."

Evidence

Existing tests commonly do a runtime null check and then legitimately use ! for non-assertion code
paths (mutation / passing an entity), where ?. cannot be used. Example: after
expect(loadedPost).to.not.be.null, the test mutates and saves via loadedPost! and
save(loadedPost!), which optional chaining cannot replace without changing behavior.

pr_compliance_checklist.yaml[20-24]
test/functional/embedded/outer-primary-column/outer-primary-column.test.ts[67-81]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new compliance rule suggests replacing non-null assertions (`!`) with optional chaining (`?.`) after null checks, but optional chaining only applies to safe access. Many tests legitimately need `!` after a null check for mutation or to pass values to functions.
### Issue Context
Repo tests already contain patterns like:
- `expect(x).to.not.be.null`
- `x!.prop = ...`
- `repo.save(x!)`
These uses of `!` are not replaceable by `?.`.
### Fix Focus Areas
- pr_compliance_checklist.yaml[20-24]
### Suggested change
Update wording so the rule only targets **property access in assertions** (the Sonar S4325 scenario) and explicitly exempts mutation/function-argument uses of `!` after a runtime null check.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Null-check style too specific~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The success criteria hardcodes expect(result).to.not.be.null, but the repo uses both
expect(x).to.not.be.null and expect(x).not.to.be.null; if the compliance checker matches
strictly, it will produce false positives for existing valid style.

Code

pr_compliance_checklist.yaml[23]

+      success_criteria: "Test assertions use expect(result).to.not.be.null followed by result?.property and result?.items ?? [] for iteration."

Evidence
The test suite contains both Chai spellings: e.g., expect(post).not.to.be.null in the transaction
isolation tests, and expect(loadedPost).to.not.be.null in embedded tests. A strict
string/pattern-based checker keyed to only one spelling will misclassify compliant code.
pr_compliance_checklist.yaml[23-23]
test/functional/transaction/database-specific-isolation/oracle-isolation.test.ts[45-52]
test/functional/embedded/outer-primary-column/outer-primary-column.test.ts[67-72]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The success criteria example uses only `expect(result).to.not.be.null`, but the repo also uses `expect(x).not.to.be.null`. If compliance enforcement is pattern-based, this will create false positives.
### Issue Context
Both spellings are valid and already used throughout the test suite.
### Fix Focus Areas
- pr_compliance_checklist.yaml[23-23]
### Suggested change
Reword success criteria to accept both forms (e.g., “`expect(x).to.not.be.null` or `expect(x).not.to.be.null`”), or make it generic (“assert `x` is not null”) rather than prescribing an exact token sequence.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

Previous review results

Review updated until commit b0a81dc

Results up to commit N/A

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. ~~Skipped loop assertions~~ ☑ 🐞 Bug ✓ Correctness

Description

The success criteria recommends result?.items ?? [] for iteration, which can cause loop bodies
(and any assertions inside) to be skipped when result is null, allowing tests to pass without
exercising checks.

Code

pr_compliance_checklist.yaml[R23-24]

+      success_criteria: "Test assertions use expect(result).to.not.be.null followed by result?.property and result?.items ?? [] for iteration."
+      failure_criteria: "Tests use result!.property or result!.items non-null assertions after null checks."

Evidence

The checklist text explicitly endorses result?.items ?? [] as a preferred iteration pattern. In
JS/TS, (null as any)?.items evaluates to undefined, and undefined ?? [] becomes [], so `for
(... of [])` performs zero iterations and silently skips any assertions that were supposed to run.

pr_compliance_checklist.yaml[20-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The compliance rule’s success criteria recommends iterating with `result?.items ?? []`, which can silently skip the loop body when `result` is `null`, allowing tests to pass without executing assertions.
### Issue Context
This is a checklist rule, so its wording will influence how reviewers/automation judge PRs. The recommended iteration pattern weakens tests by turning missing data into an empty collection.
### Fix Focus Areas
- pr_compliance_checklist.yaml[20-24]
### Suggested change
Update the success/failure criteria to avoid recommending `?? []` for iteration after a null-check expectation. Prefer either:
- using a real guard (e.g., assert non-null then use `result.items`), or
- explicitly failing when `items` is missing, rather than defaulting to `[]`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Non-null assertion overreach~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The objective/failure criteria reads like a blanket replacement of ! with ?. after null checks,
but optional chaining is not a semantic replacement for non-null assertions used for mutation or for
passing values to APIs, making the rule impractical and likely to cause noisy enforcement.

Code

pr_compliance_checklist.yaml[R22-24]

+      objective: "Tests should use optional chaining (?.) instead of non-null assertions (!) after null checks."
+      success_criteria: "Test assertions use expect(result).to.not.be.null followed by result?.property and result?.items ?? [] for iteration."
+      failure_criteria: "Tests use result!.property or result!.items non-null assertions after null checks."

Evidence

Existing tests commonly do a runtime null check and then legitimately use ! for non-assertion code
paths (mutation / passing an entity), where ?. cannot be used. Example: after
expect(loadedPost).to.not.be.null, the test mutates and saves via loadedPost! and
save(loadedPost!), which optional chaining cannot replace without changing behavior.

pr_compliance_checklist.yaml[20-24]
test/functional/embedded/outer-primary-column/outer-primary-column.test.ts[67-81]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new compliance rule suggests replacing non-null assertions (`!`) with optional chaining (`?.`) after null checks, but optional chaining only applies to safe access. Many tests legitimately need `!` after a null check for mutation or to pass values to functions.
### Issue Context
Repo tests already contain patterns like:
- `expect(x).to.not.be.null`
- `x!.prop = ...`
- `repo.save(x!)`
These uses of `!` are not replaceable by `?.`.
### Fix Focus Areas
- pr_compliance_checklist.yaml[20-24]
### Suggested change
Update wording so the rule only targets **property access in assertions** (the Sonar S4325 scenario) and explicitly exempts mutation/function-argument uses of `!` after a runtime null check.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Null-check style too specific~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The success criteria hardcodes expect(result).to.not.be.null, but the repo uses both
expect(x).to.not.be.null and expect(x).not.to.be.null; if the compliance checker matches
strictly, it will produce false positives for existing valid style.

Code

pr_compliance_checklist.yaml[23]

+      success_criteria: "Test assertions use expect(result).to.not.be.null followed by result?.property and result?.items ?? [] for iteration."

Evidence
The test suite contains both Chai spellings: e.g., expect(post).not.to.be.null in the transaction
isolation tests, and expect(loadedPost).to.not.be.null in embedded tests. A strict
string/pattern-based checker keyed to only one spelling will misclassify compliant code.
pr_compliance_checklist.yaml[23-23]
test/functional/transaction/database-specific-isolation/oracle-isolation.test.ts[45-52]
test/functional/embedded/outer-primary-column/outer-primary-column.test.ts[67-72]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The success criteria example uses only `expect(result).to.not.be.null`, but the repo also uses `expect(x).not.to.be.null`. If compliance enforcement is pattern-based, this will create false positives.
### Issue Context
Both spellings are valid and already used throughout the test suite.
### Fix Focus Areas
- pr_compliance_checklist.yaml[23-23]
### Suggested change
Reword success criteria to accept both forms (e.g., “`expect(x).to.not.be.null` or `expect(x).not.to.be.null`”), or make it generic (“assert `x` is not null”) rather than prescribing an exact token sequence.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

alumni

Qodo is not for static checks.

There are built-in eslint rules for that, which we might want to enable sometime in the future.

I would not merge this.

pkuczynski · 2026-03-23T01:00:12Z

Qodo is not for static checks.

There are built-in eslint rules for that, which we might want to enable sometime in the future.

Should we also then remove https://github.com/typeorm/typeorm/blob/master/pr_compliance_checklist.yaml#L14-L18?

And how would you like to implement in eslint a check where in the test (only!) we should first assert for not null, and then use optional chaining? I really want to see an example of such config...

qodo-free-for-open-source-projects · 2026-03-23T01:00:22Z

Review Summary by Qodo

Add optional chaining compliance rule to Qodo checklist

✨ Enhancement

Walkthroughs

Description

• Add optional chaining compliance rule to Qodo checklist
• Catches non-null assertion issues before SonarQube quality gate
• Promotes safer TypeScript patterns in test code

Diagram

flowchart LR
  A["PR Compliance Checklist"] -->|adds new rule| B["Optional Chaining Rule"]
  B -->|objective| C["Use ?. instead of !"]
  B -->|success criteria| D["expect().to.not.be.null + optional chaining"]
  B -->|failure criteria| E["Non-null assertions detected"]

File Changes

1. pr_compliance_checklist.yaml ✨ Enhancement +6/-0

Add optional chaining compliance rule

• Added new compliance rule for optional chaining in tests
• Rule enforces using optional chaining (?.) instead of non-null assertions (!)
• Includes objective, success criteria, and failure criteria for the rule
• Positioned between "Consistent Naming Conventions" and "Remove AI-generated noise" rules

pr_compliance_checklist.yaml

qodo-free-for-open-source-projects · 2026-03-23T01:03:32Z

Persistent review updated to latest commit d731fae

alumni · 2026-03-24T13:18:29Z

How about eslint: @typescript-eslint/prefer-optional-chain. Might as well add @typescript-eslint/prefer-nullish-coalescing since it's similar.

Normally we would add the entire config at once (@typescript-eslint/stylistic-type-checked), but our code base might not be there yet (the internal types are not always correct, so some rule that might be stylistic based on the type information, could end up breaking functionality)

I also noticed there's a lot of .substr(0, 1) which could be fixed with @typescript-eslint/prefer-string-starts-ends-with, in the same config.

Sonar also complains quite a lot about unnecessary type assertions, e.g. @typescript-eslint/no-unnecessary-type-assertion - this is in the recommended config, but was disabled on purpose since removing the assertions would cause more trouble than keeping them. This one in particular we should delay enabling for a while - especially with the new TS version (v6 was just released and we can't upgrade yet).

Should we also then remove https://github.com/typeorm/typeorm/blob/master/pr_compliance_checklist.yaml#L14-L18?

That would great, that Qodo rule is so generic that I doubt is of any use. "project's established naming standards" - what's that mean? They are not described anywhere. @typescript-eslint/naming-convention is the way.

And how would you like to implement in eslint a check where in the test (only!) we should first assert for not null, and then use optional chaining?

typescript-eslint uses the inferred types in each line. So if the assertion is properly typed, you'd get the right inference. Unfortunately, chai isn't that great. Inference is good with assert assertions, but we tend to use expect and should assertions which are closer to natural language.

…ning-compliance

cloudflare-workers-and-pages · 2026-03-29T12:51:29Z

Deploying typeorm with Cloudflare Pages

Latest commit:	`e8e4aa9`
Status:	✅ Deploy successful!
Preview URL:	https://73271202.typeorm.pages.dev
Branch Preview URL:	https://chore-optional-chaining-comp.typeorm.pages.dev

View logs

qodo-free-for-open-source-projects · 2026-03-29T12:53:27Z

Persistent review updated to latest commit e8e4aa9

…t both null-check styles

sonarqubecloud · 2026-03-29T12:58:14Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

qodo-free-for-open-source-projects · 2026-03-29T12:59:15Z

Persistent review updated to latest commit b0a81dc

pkuczynski · 2026-04-09T22:30:46Z

I've been enabling these rules incrementally in separate PRs:

@typescript-eslint/prefer-optional-chain — style(eslint): enable prefer-optional-chain rule #12331 (fixed 150 violations)
@typescript-eslint/prefer-string-starts-ends-with — style(eslint): enable prefer-string-starts-ends-with rule #12332 (codebase already clean, enabled as guard rail)

@typescript-eslint/prefer-nullish-coalescing was attempted but dropped — 642 violations that can't be auto-fixed due to intentional || usage with falsy primitives.

I also tackled the non-null assertion issue from SonarQube by replacing findOne/getOne with OrFail variants across the test suite in #12325.

Closing in favor of the PRs above.

ci(qodo): add optional chaining compliance rule

d731fae

pkuczynski self-assigned this Mar 22, 2026

pkuczynski requested review from alumni, gioboa and naorpeled March 22, 2026 22:24

pkuczynski added this to the 1.0 milestone Mar 22, 2026

pkuczynski enabled auto-merge (squash) March 22, 2026 22:28

naorpeled approved these changes Mar 22, 2026

View reviewed changes

alumni reviewed Mar 22, 2026

View reviewed changes

alumni disabled auto-merge March 22, 2026 23:03

alumni closed this Mar 22, 2026

pkuczynski reopened this Mar 23, 2026

Merge remote-tracking branch 'origin/master' into chore/optional-chai…

e8e4aa9

…ning-compliance

fix(qodo): refine optional chaining rule to exempt mutation and accep…

b0a81dc

…t both null-check styles

gioboa approved these changes Apr 3, 2026

View reviewed changes

This was referenced Apr 8, 2026

refactor(test): replace findOne with findOneOrFail to eliminate non-null assertions #12325

Merged

style(eslint): enable prefer-optional-chain rule #12331

Merged

style(eslint): enable prefer-string-starts-ends-with rule #12332

Merged

pkuczynski closed this Apr 9, 2026

pkuczynski deleted the chore/optional-chaining-compliance branch April 9, 2026 22:50

pkuczynski mentioned this pull request Apr 10, 2026

style(eslint): enable prefer-nullish-coalescing rule #12340

Merged

Navigation Menu

Repository navigation

ci(qodo): add optional chaining compliance rule - #12255

ci(qodo): add optional chaining compliance rule#12255
pkuczynski wants to merge 3 commits into
masterfrom
chore/optional-chaining-compliance

pkuczynski commented Mar 22, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 22, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 22, 2026 •

edited

Loading

Uh oh!

alumni left a comment •

edited

Loading

Uh oh!

pkuczynski commented Mar 23, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 23, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 23, 2026

Uh oh!

alumni commented Mar 24, 2026 •

edited

Loading

Uh oh!

cloudflare-workers-and-pages Bot commented Mar 29, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 29, 2026

Uh oh!

sonarqubecloud Bot commented Mar 29, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 29, 2026

Uh oh!

pkuczynski commented Apr 9, 2026 •

edited

Loading

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Conversation

pkuczynski commented Mar 22, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 22, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

alumni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkuczynski commented Mar 23, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 23, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 23, 2026

Uh oh!

alumni commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pages Bot commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying typeorm with Cloudflare Pages

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 29, 2026

Uh oh!

sonarqubecloud Bot commented Mar 29, 2026

Quality Gate passed

Uh oh!

qodo-free-for-open-source-projects Bot commented Mar 29, 2026

Uh oh!

pkuczynski commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

qodo-free-for-open-source-projects Bot commented Mar 22, 2026 •

edited

Loading

alumni left a comment •

edited

Loading

alumni commented Mar 24, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Mar 29, 2026 •

edited

Loading

pkuczynski commented Apr 9, 2026 •

edited

Loading