Skip to content

Commit 521f026

Browse files
tnaum-mstnaum-ms
committed
Fix nested and $-prefixed field references; add edge-case tests and PR notes
- buildAggregationReference() emits { $getField: { field, input } } for unsafe segments in nested paths (was a broken top-level lookup) - stricter AGGREGATION_SAFE_SEGMENT_PATTERN routes $-containing names (e.g. $price) through $getField instead of emitting $$price - tests for nested unsafe leaf, safe-prefix collapse, mixed segments, $-in-name, literal-dot ambiguity, and empty/degenerate segments - mark future-work.md item 2 resolved; add docs/ai-and-plans/PRs note
1 parent 5a01e80 commit 521f026

4 files changed

Lines changed: 389 additions & 20 deletions

File tree

‎docs/ai-and-plans/PRs/717-referenceText-unsafe-field-names.md‎

Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
# PR #717: Correct MQL aggregation references for unsafe field names
2+
3+
**Branch:** `fix/709-referenceText-unsafe-field-names`
4+
**Base:** `main`
5+
**Date:** 2026-06-23
6+
**PR URL:** https://github.com/microsoft/vscode-documentdb/pull/717
7+
**Fixes:** #709
8+
9+
---
10+
11+
## Why
12+
13+
`toFieldCompletionItems()` produces a `referenceText` for each schema field — the
14+
aggregation field reference (e.g. `$age`) that a future aggregation completion
15+
provider will offer. The original implementation always emitted `"$" + path`,
16+
which is **invalid MQL** for field names that are not valid `$`-prefix references:
17+
18+
| Field name | Old `referenceText` | Valid? |
19+
| --------------- | ------------------- | ------ |
20+
| `order-items` | `$order-items` | ❌ |
21+
| `my field` | `$my field` | ❌ |
22+
| `say"hi"` | `$say"hi"` | ❌ |
23+
24+
This was documented as future work in
25+
[`future-work.md`](../../../src/utils/json/data-api/autocomplete/future-work.md)
26+
(item 2), with `$getField` (Option B) recommended as the fix.
27+
28+
An initial pass added a single-segment `$getField` fallback, but it had three
29+
correctness gaps that this PR closes:
30+
31+
1. **Nested paths were broken.** `a.order-items` produced
32+
`{ $getField: "a.order-items" }`, which references a *top-level* field
33+
literally named `a.order-items` — the wrong document — instead of the nested
34+
`order-items` field inside `a`.
35+
2. **`$`-containing names were misclassified as safe.** The identifier check
36+
allowed `$`, so a field named `$price` emitted `$$price`, which MQL reads as
37+
the **variable** `$$price`, not the field.
38+
3. The interface doc claimed nested paths were handled when they were not.
39+
40+
### Scope cleanup (rebase)
41+
42+
The branch was originally stacked on the contributor's `#659` (index-count)
43+
branch, so the PR carried unrelated `IndexesItem.ts` changes that duplicated a
44+
feature **already merged to `main`** — the source of the merge conflict. The
45+
branch was rebased onto `main`, dropping the stale `#659` commits and keeping
46+
only the `#709` work. The PR now changes four files: the two autocomplete
47+
sources ([`toFieldCompletionItems.ts`](../../../src/utils/json/data-api/autocomplete/toFieldCompletionItems.ts)
48+
and its test), the [`future-work.md`](../../../src/utils/json/data-api/autocomplete/future-work.md)
49+
status update, and this note.
50+
51+
---
52+
53+
## What was done
54+
55+
### 1. Stricter aggregation-safe segment check
56+
57+
A new `AGGREGATION_SAFE_SEGMENT_PATTERN` (`/^[a-zA-Z_][a-zA-Z0-9_]*$/`) replaces
58+
the previous reuse of `JS_IDENTIFIER_PATTERN` for reference safety. It is
59+
intentionally stricter: it rejects `$` because `$`/`$$` are reserved in
60+
aggregation expressions. `JS_IDENTIFIER_PATTERN` is still used unchanged for
61+
`insertText` quoting (a separate concern).
62+
63+
### 2. `buildAggregationReference(path)` — correct nested references
64+
65+
```
66+
$a.b.c ← every segment safe (compact form)
67+
{ $getField: "order-items" } ← unsafe top-level field
68+
{ $getField: { field: "order-items", ← unsafe leaf in a nested path
69+
input: "$a" } }
70+
```
71+
72+
- A fully safe path keeps the compact `$a.b.c` form (unchanged behavior).
73+
- Otherwise the reference is built with `$getField`. The leading run of safe
74+
segments is collapsed into a single quoted `"$a.b"` field-path used as the
75+
innermost `input`, and each subsequent (unsafe or trailing) segment is wrapped
76+
in `{ $getField: { field, input } }`. This is what makes nested references
77+
semantically correct.
78+
- All literal segment names are escaped via the existing `escapeFieldName`
79+
(`\` → `\\`, `"` → `\"`).
80+
81+
### 3. Documentation
82+
83+
- The `referenceText` JSDoc now describes the `$getField` forms and the
84+
literal-dot limitation.
85+
- `future-work.md` item 2 is marked ✅ RESOLVED (mirroring item 1), with the
86+
original description preserved in a `<details>` block.
87+
88+
---
89+
90+
## Where this applies (scope)
91+
92+
`referenceText` is the **aggregation field-path expression** (`$field`) form. It
93+
is used wherever a field appears as an _expression value_, not as a _key_:
94+
95+
```
96+
✔ affected (uses referenceText / "$field" form)
97+
• db.coll.aggregate([ { $project / $group / $addFields / $match: { $expr } ... } ])
98+
• find / update with $expr: find({ $expr: { $gt: ["$order-items", 5] } })
99+
• pipeline-form updates: updateOne(filter, [ { $set: { x: "$a.order-items" } } ])
100+
101+
✘ NOT affected (uses insertText — the quoted key, a different field)
102+
• plain find filters: { "order-items": 5 }
103+
• projection include/exclude: { "order-items": 1 }
104+
• index keys, sort keys, etc. { "order-items": -1 }
105+
```
106+
107+
In a query a field is either a **key** (left of `:`) → `insertText`, unchanged by
108+
this PR — or an **expression value** (a `$`-reference) → `referenceText`, fixed
109+
here. So this is effectively "aggregation-expression contexts" (pipelines plus
110+
`$expr` and pipeline-style updates), not plain field-name positions.
111+
112+
---
113+
114+
## How it surfaces (examples)
115+
116+
### Sample collection
117+
118+
```
119+
db.orders — a document looks like:
120+
┌─────────────────────────────────────────────┐
121+
│ { │
122+
│ "age": 30, │
123+
│ "address": { "city": "Berlin" }, │
124+
│ "order-items": [ ... ], ← dash (unsafe) │
125+
│ "a": { "order-items": 5 }, ← nested│
126+
│ "$price": 9.99 ← leading $ │
127+
│ } │
128+
└─────────────────────────────────────────────┘
129+
```
130+
131+
### Where the completion kicks in
132+
133+
The cursor sits where an **expression** is expected, and the provider lists fields:
134+
135+
```
136+
db.orders.aggregate([
137+
{ $project: { total: ▮ } } ◄── cursor here, provider lists fields
138+
])
139+
│
140+
▼
141+
┌──────────────────────────┐
142+
│ ▸ age number │
143+
│ ▸ address.city string │
144+
│ ▸ order-items array │ ◄ pick one →
145+
│ ▸ a.order-items number │ inserts its
146+
│ ▸ $price number │ referenceText
147+
└──────────────────────────┘
148+
```
149+
150+
### Before vs. after (what gets inserted)
151+
152+
```
153+
field picked BEFORE (always "$"+path) AFTER (this PR)
154+
────────────────── ────────────────────────────── ─────────────────────────────────────────────────
155+
age $age ✅ $age ✅ (unchanged)
156+
address.city $address.city ✅ $address.city ✅ (unchanged)
157+
158+
order-items $order-items ❌ { $getField: "order-items" } ✅
159+
└ parsed as $order MINUS items
160+
161+
a.order-items $a.order-items ❌ { $getField: { field: "order-items", ✅
162+
└ "$" form breaks on dash input: "$a" } }
163+
└ walks INTO "a", then reads the leaf
164+
165+
$price $$price ❌ { $getField: "$price" } ✅
166+
└ "$$" = a VARIABLE, not a field └ name treated as a literal
167+
```
168+
169+
### The subtle one — nested vs. top-level (the core fix)
170+
171+
The earlier single-segment patch would have emitted `{ $getField: "a.order-items" }`,
172+
which is still wrong: `$getField` with a plain string reads a _top-level_ field
173+
literally named `a.order-items`.
174+
175+
```
176+
{ $getField: "a.order-items" } { $getField: { field: "order-items", input: "$a" } }
177+
┌───────────── ROOT ──────────────┐ ┌───────────── ROOT ──────────────┐
178+
│ looks for a key literally named │ │ input: $a ──► { order-items: 5}│
179+
│ "a.order-items" → not found │ ✗ │ field: "order-items" ──► 5 │ ✓
180+
└──────────────────────────────────┘ └──────────────────────────────────┘
181+
```
182+
183+
---
184+
185+
## Edge cases covered (tests)
186+
187+
All in
188+
[`toFieldCompletionItems.test.ts`](../../../src/utils/json/data-api/autocomplete/toFieldCompletionItems.test.ts):
189+
190+
| Case | Input | `referenceText` |
191+
| ------------------------------------- | ---------------- | -------------------------------------------------------------------------------- |
192+
| Safe scalar / nested (regression) | `age`, `address.city` | `$age`, `$address.city` |
193+
| Unsafe top-level | `order-items` | `{ $getField: "order-items" }` |
194+
| Embedded quotes (top-level) | `say"hi"` | `{ $getField: "say\"hi\"" }` |
195+
| **Unsafe leaf in nested path** | `a.order-items` | `{ $getField: { field: "order-items", input: "$a" } }` |
196+
| **Safe multi-segment prefix collapse**| `a.b.c-d` | `{ $getField: { field: "c-d", input: "$a.b" } }` |
197+
| **Unsafe segment then safe segment** | `order-items.city` | `{ $getField: { field: "city", input: { $getField: "order-items" } } }` |
198+
| **Embedded quotes (nested)** | `a.say"hi"` | `{ $getField: { field: "say\"hi\"", input: "$a" } }` |
199+
| **Case 3 — `$` in name** | `$price`, `a$b`, `a.$inner` | `{ $getField: "$price" }`, `{ $getField: "a$b" }`, `{ $getField: { field: "$inner", input: "$a" } }` |
200+
| **Case 4 — literal-dot ambiguity** | `a.b` | `$a.b` (treated as nested — documented limitation) |
201+
| **Case 5 — empty / degenerate** | `""`, `a..b` | `{ $getField: "" }`, nested `$getField` chain (defensive; never emitted by `getKnownFields`) |
202+
203+
### Known limitation (out of scope)
204+
205+
A field literally named `"a.b"` is indistinguishable from a nested `{ a: { b } }`
206+
because `FieldEntry.path` is a flattened, dot-joined string. Dots are always
207+
interpreted as nesting. Resolving this requires changing `path` to a segment
208+
array — tracked as item 3 in `future-work.md`.
209+
210+
### Why no production-behavior risk
211+
212+
`referenceText` is currently consumed only by tests; no production code path
213+
reads it yet (the aggregation completion provider is not wired up). This change
214+
prepares correct data for that future provider with no user-facing behavior
215+
change today.

‎src/utils/json/data-api/autocomplete/future-work.md‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,23 @@ Tests cover dashes, brackets, digits, embedded quotes, and backslashes.
1515

1616
---
1717

18-
## 2. `referenceText` is invalid MQL for special field names
18+
## 2. ~~`referenceText` is invalid MQL for special field names~~ ✅ RESOLVED
19+
20+
**Resolved in:** PR #717 (fixes #709)
21+
22+
Implemented Option B (`$getField`) with full support for nested paths via the
23+
`{ $getField: { field, input } }` form, plus a stricter aggregation-safe segment
24+
check that also routes `$`-containing names through `$getField`:
25+
26+
- top-level unsafe field → `{ $getField: "order-items" }`
27+
- nested unsafe segment → `{ $getField: { field: "order-items", input: "$a" } }`
28+
- `$`-prefixed name (e.g. `$price`) → `{ $getField: "$price" }` (would otherwise be misread as the variable `$$price`)
29+
30+
Literal-dot field names remain a known limitation (see item 3) because `path` is
31+
a flattened string; dots are always interpreted as nesting.
32+
33+
<details>
34+
<summary>Original description</summary>
1935

2036
**Severity:** Medium — will generate broken aggregation expressions
2137
**File:** `toFieldCompletionItems.ts` — `referenceText` construction
@@ -51,6 +67,8 @@ referenceText: needsQuoting
5167

5268
**Recommendation:** Option B is pragmatic. Option C is more flexible if we later need to support both forms in different contexts (e.g., `$match` vs `$project`).
5369

70+
</details>
71+
5472
---
5573

5674
## 3. `FieldEntry.path` dot-concatenation is ambiguous for literal dots

‎src/utils/json/data-api/autocomplete/toFieldCompletionItems.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,86 @@ describe('toFieldCompletionItems', () => {
113113
expect(result[0].referenceText).toBe('{ $getField: "say\\"hi\\"" }');
114114
});
115115

116+
// Multi-segment paths with an unsafe segment: a flat
117+
// `{ $getField: "a.order-items" }` would look up a *top-level* field
118+
// literally named "a.order-items", which is the wrong document. The
119+
// correct reference walks into the parent via `{ field, input }`.
120+
it('uses { field, input } for an unsafe leaf in a nested path', () => {
121+
const fields: FieldEntry[] = [{ path: 'a.order-items', type: 'string', bsonType: 'string' }];
122+
123+
const result = toFieldCompletionItems(fields);
124+
125+
expect(result[0].referenceText).toBe('{ $getField: { field: "order-items", input: "$a" } }');
126+
});
127+
128+
it('collapses a multi-segment safe prefix into a single $a.b input', () => {
129+
const fields: FieldEntry[] = [{ path: 'a.b.c-d', type: 'string', bsonType: 'string' }];
130+
131+
const result = toFieldCompletionItems(fields);
132+
133+
expect(result[0].referenceText).toBe('{ $getField: { field: "c-d", input: "$a.b" } }');
134+
});
135+
136+
it('nests $getField when an unsafe segment is followed by safe segments', () => {
137+
const fields: FieldEntry[] = [{ path: 'order-items.city', type: 'string', bsonType: 'string' }];
138+
139+
const result = toFieldCompletionItems(fields);
140+
141+
expect(result[0].referenceText).toBe('{ $getField: { field: "city", input: { $getField: "order-items" } } }');
142+
});
143+
144+
it('escapes embedded quotes in nested $getField segments', () => {
145+
const fields: FieldEntry[] = [{ path: 'a.say"hi"', type: 'string', bsonType: 'string' }];
146+
147+
const result = toFieldCompletionItems(fields);
148+
149+
expect(result[0].referenceText).toBe('{ $getField: { field: "say\\"hi\\"", input: "$a" } }');
150+
});
151+
152+
// Case 3: a field name beginning with `$` is NOT a valid `$`-prefix
153+
// reference (it would be read as a variable, e.g. `$$price`). It must
154+
// fall back to `$getField`, where the name is an opaque literal.
155+
it('uses $getField for field names containing a $ (would be misread as a variable)', () => {
156+
const fields: FieldEntry[] = [
157+
{ path: '$price', type: 'number', bsonType: 'int32' },
158+
{ path: 'a$b', type: 'string', bsonType: 'string' },
159+
{ path: 'a.$inner', type: 'string', bsonType: 'string' },
160+
];
161+
162+
const result = toFieldCompletionItems(fields);
163+
164+
expect(result[0].referenceText).toBe('{ $getField: "$price" }');
165+
expect(result[1].referenceText).toBe('{ $getField: "a$b" }');
166+
expect(result[2].referenceText).toBe('{ $getField: { field: "$inner", input: "$a" } }');
167+
});
168+
169+
// Case 4: with a flattened `path` string there is no way to tell a field
170+
// literally named "a.b" from a nested `{ a: { b } }`. Dots are always
171+
// treated as nesting, so a safe dotted path keeps the compact form.
172+
it('treats a dotted path with safe segments as nested (literal-dot ambiguity)', () => {
173+
const fields: FieldEntry[] = [{ path: 'a.b', type: 'string', bsonType: 'string' }];
174+
175+
const result = toFieldCompletionItems(fields);
176+
177+
expect(result[0].referenceText).toBe('$a.b');
178+
});
179+
180+
// Case 5 (defensive): getKnownFields never emits empty segments, but the
181+
// builder must not produce a broken bare `$` reference if it ever does.
182+
it('does not emit a bare $ reference for empty or degenerate segments', () => {
183+
const fields: FieldEntry[] = [
184+
{ path: '', type: 'string', bsonType: 'string' },
185+
{ path: 'a..b', type: 'string', bsonType: 'string' },
186+
];
187+
188+
const result = toFieldCompletionItems(fields);
189+
190+
expect(result[0].referenceText).toBe('{ $getField: "" }');
191+
expect(result[1].referenceText).toBe(
192+
'{ $getField: { field: "b", input: { $getField: { field: "", input: "$a" } } } }',
193+
);
194+
});
195+
116196
it('preserves isSparse', () => {
117197
const fields: FieldEntry[] = [
118198
{ path: 'name', type: 'string', bsonType: 'string', isSparse: false },

0 commit comments

Comments
 (0)