Skip to content

fix(deno-kv): hasItem answers presence, not the value's truthiness - #869

Closed
superbot-helper wants to merge 1 commit into
unjs:mainfrom
superbot-helper:fix/deno-kv-hasitem-empty
Closed

superbot-helper wants to merge 1 commit into
unjs:mainfrom
superbot-helper:fix/deno-kv-hasitem-empty

Conversation

@superbot-helper

@superbot-helper superbot-helper commented Nov 3, 2026 •

Copy link
Copy Markdown

What happens

hasItem returned !!value.value — the value's truthiness, not the entry's presence. Every stringified value is truthy except one: the empty string, which is a legal stored value because stringify("") is "".

So the two methods disagreed about the same key:

deno-kv   empty    hasItem=false   getItem=""     <- contradict itself
deno-kv   zero     hasItem=true    getItem=0
deno-kv   false    hasItem=true    getItem=false
deno-kv   missing  hasItem=false   getItem=null
memory    empty    hasItem=true    getItem=""

memory, localstorage, fs and indexedb all answer true for that key. deno-kv-node spreads this driver and inherits the bug.

The change

Deno KV answers a missing key with value: null, so presence is value.value !== null. One line.

After, with the same fake openKv injected through the driver's own option:

deno-kv   empty    hasItem=true    getItem=""

Why the suite missed it

The shared harness has a falsy-value test for getItem only (test/drivers/utils.ts:176), not for hasItem, and deno-kv's own tests run against a real KV binding that CI cannot always provide.

Not a duplicate

Seven open pull requests mention hasItem — three are mine from this sitting (#866, #867, #868) and the rest are features. Four mention deno-kv; none is this. Searched by symptom across issues and pull requests, open and closed.

Scope

deno-kv and, through its spread, deno-kv-node. No other driver, and no change to getItem.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected item checks so entries containing empty strings or other falsy values are recognized as present.

`!!value.value` is false for a key holding the empty string, which is a value
the driver stores like any other: stringify("") is "". So hasItem and getItem
disagreed about the same key, and deno-kv-node inherits it through its spread.

Deno KV answers a missing key with value null, so the presence test is that.
@vercel

vercel Bot commented Nov 3, 2026

Copy link
Copy Markdown

@superbot-helper is attempting to deploy a commit to the unjs Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

hasItem now reports Deno KV entries with falsy values as present. It checks whether the stored value is non-null.

Changes: Deno KV presence check

Layer / File(s) Summary
Check stored value presence
src/drivers/deno-kv.ts
hasItem checks whether the entry value is non-null. Stored falsy values therefore count as present.

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Suggested reviewers: pi0

Merge Risk: Minimal · up to 6863d

The change correctly treats falsy stored values as present. No actionable merge-blocking risk is identified; merge after normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the change accurately. The PR changes the deno-kv driver so that hasItem checks whether a stored value is present (not null) instead of testing its truthiness, which matches the raw su…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the change accurately. The PR changes the deno-kv driver so that hasItem checks whether a stored value is present (not null) instead of testing its truthiness, which matches the raw summary and objectives. The title uses the conventional fix(scope) form and names the affected driver and method, so it is specific enough for a teammate scanning history.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5f4d03d4-5301-45cb-bc8c-8d130ed40317
Commits

Reviewing files that changed from the base of the PR and between 5781aae and 6863d73.

Files selected for processing: 1
  • src/drivers/deno-kv.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.

@unjs unjs temporarily blocked superbot-helper Nov 3, 2026
@pi0 pi0 closed this Nov 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants