Merge pull request #17788 from prometheus/fix-identifier-completion-end

Replace entire identifier when autocompleting inside of it
3 days ago · ffcdb2bc1f
parent 0a2be81616 fe76e6c297
commit ffcdb2bc1f
2 changed files with 204 additions and 7 deletions
--- a/web/ui/module/codemirror-promql/src/complete/hybrid.test.ts
+++ b/web/ui/module/codemirror-promql/src/complete/hybrid.test.ts
@ -11,7 +11,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-import { analyzeCompletion, computeStartCompletePosition, ContextKind, durationWithUnitRegexp } from './hybrid';
+import { analyzeCompletion, computeStartCompletePosition, computeEndCompletePosition, ContextKind, durationWithUnitRegexp } from './hybrid';
 import { createEditorState, mockedMetricsTerms, mockPrometheusServer } from '../test/utils-test';
 import { Completion, CompletionContext } from '@codemirror/autocomplete';
 import {
@ -638,7 +638,7 @@ describe('analyzeCompletion test', () => {
      const state = createEditorState(value.expr);
      const node = syntaxTree(state).resolve(value.pos, -1);
      const result = analyzeCompletion(state, node, value.pos);
-      expect(value.expectedContext).toEqual(result);
+      expect(result).toEqual(value.expectedContext);
    });
  });
 });
@ -861,7 +861,146 @@ describe('computeStartCompletePosition test', () => {
      const state = createEditorState(value.expr);
      const node = syntaxTree(state).resolve(value.pos, -1);
      const result = computeStartCompletePosition(state, node, value.pos);
-      expect(value.expectedStart).toEqual(result);
+      expect(result).toEqual(value.expectedStart);
+    });
+  });
+});
+
+describe('computeEndCompletePosition test', () => {
+  const testCases = [
+    {
+      title: 'cursor at end of metric name',
+      expr: 'metric_name',
+      pos: 11, // cursor is at the end
+      expectedEnd: 11,
+    },
+    {
+      title: 'cursor in middle of metric name - should extend to end',
+      expr: 'coredns_cache_hits_total',
+      pos: 14, // cursor is after 'coredns_cache_' (before 'hits')
+      expectedEnd: 24, // should extend to end of 'coredns_cache_hits_total'
+    },
+    {
+      title: 'cursor in middle of metric name inside rate() - should extend to end',
+      expr: 'rate(coredns_cache_hits_total[2m])',
+      pos: 19, // cursor is after 'coredns_cache_' (before 'hits')
+      expectedEnd: 29, // should extend to end of 'coredns_cache_hits_total'
+    },
+    {
+      title: 'cursor in middle of metric name inside sum(rate()) - should extend to end',
+      expr: 'sum(rate(coredns_cache_hits_total[2m]))',
+      pos: 24, // cursor is after 'coredns_cache_' (before 'hits')
+      expectedEnd: 33, // should extend to end of 'coredns_cache_hits_total'
+    },
+    {
+      title: 'cursor at beginning of metric name - should extend to end',
+      expr: 'metric_name',
+      pos: 1, // cursor after 'm'
+      expectedEnd: 11,
+    },
+    {
+      title: 'cursor in middle of incomplete function name - should extend to end',
+      expr: 'sum_ov',
+      pos: 4, // cursor after 'sum_' (before 'ov')
+      expectedEnd: 6, // should extend to end of 'sum_ov'
+    },
+    {
+      title: 'cursor in middle of incomplete function name within aggregator - should extend to end',
+      expr: 'sum(sum_ov(foo[5m]))',
+      pos: 8, // cursor after 'sum_' (before 'ov')
+      expectedEnd: 10, // should extend to end of 'sum_ov'
+    },
+    {
+      title: 'empty bracket - ends before the closing bracket',
+      expr: '{}',
+      pos: 1,
+      expectedEnd: 1,
+    },
+    {
+      title: 'cursor in label matchers - ends before the closing bracket',
+      expr: 'metric_name{label="value"}',
+      pos: 12, // cursor after '{'
+      expectedEnd: 25,
+    },
+    {
+      title: 'cursor in middle of label name in grouping clause - should extend to end',
+      expr: 'sum by (instance_name)',
+      pos: 12, // cursor after 'inst' (before 'ance')
+      expectedEnd: 21, // should extend to end of 'instance_name'
+    },
+    {
+      title: 'cursor in middle of label name in label matcher - should extend to end',
+      expr: 'metric{instance_name="value"}',
+      pos: 11, // cursor after 'inst' (before 'ance')
+      expectedEnd: 20, // should extend to end of 'instance_name'
+    },
+    {
+      title: 'cursor in middle of label name in on() modifier - should extend to end',
+      expr: 'a / on(instance_name) b',
+      pos: 11, // cursor after 'inst' (before 'ance')
+      expectedEnd: 20, // should extend to end of 'instance_name'
+    },
+    {
+      title: 'cursor in middle of label name in ignoring() modifier - should extend to end',
+      expr: 'a / ignoring(instance_name) b',
+      pos: 17, // cursor after 'inst' (before 'ance')
+      expectedEnd: 26, // should extend to end of 'instance_name'
+    },
+    {
+      title: 'cursor in middle of function name rate - should extend to end',
+      expr: 'rate(foo[5m])',
+      pos: 2, // cursor after 'ra' (before 'te')
+      expectedEnd: 4, // should extend to end of 'rate'
+    },
+    {
+      title: 'cursor in middle of function name histogram_quantile - should extend to end',
+      expr: 'histogram_quantile(0.9, rate(foo[5m]))',
+      pos: 10, // cursor after 'histogram_' (before 'quantile')
+      expectedEnd: 18, // should extend to end of 'histogram_quantile'
+    },
+    {
+      title: 'cursor in middle of aggregator sum - should extend to end',
+      expr: 'sum(rate(foo[5m]))',
+      pos: 2, // cursor after 'su' (before 'm')
+      expectedEnd: 3, // should extend to end of 'sum'
+    },
+    {
+      title: 'cursor in middle of aggregator count_values - should extend to end',
+      expr: 'count_values("label", foo)',
+      pos: 6, // cursor after 'count_' (before 'values')
+      expectedEnd: 12, // should extend to end of 'count_values'
+    },
+    {
+      title: 'cursor in middle of nested function - should extend to end',
+      expr: 'sum(rate(foo[5m]))',
+      pos: 6, // cursor after 'ra' inside rate (before 'te')
+      expectedEnd: 8, // should extend to end of 'rate'
+    },
+    {
+      title: 'cursor at beginning of aggregator - should extend to end',
+      expr: 'avg by (instance) (rate(foo[5m]))',
+      pos: 1, // cursor after 'a' (before 'vg')
+      expectedEnd: 3, // should extend to end of 'avg'
+    },
+    {
+      title: 'cursor in middle of function name with binary op - should extend to end',
+      expr: 'rate(foo[5m]) / irate(bar[5m])',
+      pos: 17, // cursor after 'ir' inside irate (before 'ate')
+      expectedEnd: 21, // should extend to end of 'irate'
+    },
+    {
+      title: 'error node - returns pos (cursor position)',
+      expr: 'metric_name !',
+      pos: 13, // cursor at '!' (error node)
+      expectedEnd: 13, // error node returns pos
+    },
+  ];
+  testCases.forEach((value) => {
+    it(value.title, () => {
+      const state = createEditorState(value.expr);
+      const node = syntaxTree(state).resolve(value.pos, -1);
+      const result = computeEndCompletePosition(node, value.pos);
+      expect(result).toEqual(value.expectedEnd);
    });
  });
 });
@ -915,6 +1054,28 @@ describe('autocomplete promQL test', () => {
        validFor: /^[a-zA-Z0-9_:]+$/,
      },
    },
+    {
+      title: 'cursor in middle of metric name - to should extend to end (issue #15839)',
+      expr: 'sum(coredns_cache_hits_total)',
+      pos: 18, // cursor is after 'coredns_cache_' (before 'hits')
+      expectedResult: {
+        options: ([] as Completion[]).concat(functionIdentifierTerms, aggregateOpTerms, snippets),
+        from: 4,
+        to: 28, // should extend to end of 'coredns_cache_hits_total'
+        validFor: /^[a-zA-Z0-9_:]+$/,
+      },
+    },
+    {
+      title: 'cursor in middle of metric name inside rate() - to should extend to end (issue #15839)',
+      expr: 'rate(coredns_cache_hits_total[2m])',
+      pos: 19, // cursor is after 'coredns_cache_' (before 'hits')
+      expectedResult: {
+        options: ([] as Completion[]).concat(functionIdentifierTerms, aggregateOpTerms, snippets),
+        from: 5,
+        to: 29, // should extend to end of 'coredns_cache_hits_total'
+        validFor: /^[a-zA-Z0-9_:]+$/,
+      },
+    },
    {
      title: 'offline function/aggregation autocompletion in aggregation 3',
      expr: 'sum(rate())',
@ -1285,7 +1446,7 @@ describe('autocomplete promQL test', () => {
      expectedResult: {
        options: [],
        from: 10,
-        to: 10,
+        to: 11,
        validFor: /^[a-zA-Z0-9_:]+$/,
      },
    },
@ -1296,7 +1457,7 @@ describe('autocomplete promQL test', () => {
      expectedResult: {
        options: [],
        from: 10,
-        to: 10,
+        to: 12,
        validFor: /^[a-zA-Z0-9_:]+$/,
      },
    },
@ -1451,7 +1612,7 @@ describe('autocomplete promQL test', () => {
      const context = new CompletionContext(state, value.pos, true);
      const completion = newCompleteStrategy(value.conf);
      const result = await completion.promQL(context);
-      expect(value.expectedResult).toEqual(result);
+      expect(result).toEqual(value.expectedResult);
    });
  });

--- a/web/ui/module/codemirror-promql/src/complete/hybrid.ts
+++ b/web/ui/module/codemirror-promql/src/complete/hybrid.ts
@ -166,6 +166,36 @@ function arrayToCompletionResult(data: Completion[], from: number, to: number, i
  } as CompletionResult;
 }

+// computeEndCompletePosition calculates the end position for autocompletion replacement.
+// When the cursor is in the middle of a token, this ensures the entire token is replaced,
+// not just the portion before the cursor. This fixes issue #15839.
+// Note: this method is exported only for testing purpose.
+export function computeEndCompletePosition(node: SyntaxNode, pos: number): number {
+  // For error nodes, use the cursor position as the end position
+  if (node.type.id === 0) {
+    return pos;
+  }
+
+  if (
+    node.type.id === LabelMatchers ||
+    node.type.id === GroupingLabels ||
+    node.type.id === FunctionCallBody ||
+    node.type.id === MatrixSelector ||
+    node.type.id === SubqueryExpr
+  ) {
+    // When we're inside empty brackets, we want to replace up to just before the closing bracket.
+    return node.to - 1;
+  }
+
+  if (node.type.id === StringLiteral && (node.parent?.type.id === UnquotedLabelMatcher || node.parent?.type.id === QuotedLabelMatcher)) {
+    // For label values, we want to replace all content inside the quotes.
+    return node.parent.to - 1;
+  }
+
+  // For all other nodes, extend the end position to include the entire token.
+  return node.to;
+}
+
 // Matches complete PromQL durations, including compound units (e.g., 5m, 1d2h, 1h30m, etc.).
 // Duration units are a fixed, safe set (no regex metacharacters), so no escaping is needed.
 export const durationWithUnitRegexp = new RegExp(`^(\\d+(${durationTerms.map((term) => term.label).join('|')}))+$`);
@ -667,7 +697,13 @@ export class HybridComplete implements CompleteStrategy {
      }
    }
    return asyncResult.then((result) => {
-      return arrayToCompletionResult(result, computeStartCompletePosition(state, tree, pos), pos, completeSnippet, span);
+      return arrayToCompletionResult(
+        result,
+        computeStartCompletePosition(state, tree, pos),
+        computeEndCompletePosition(tree, pos),
+        completeSnippet,
+        span
+      );
    });
  }