evidence-dev / evidence

Business intelligence as code: build fast, interactive data visualizations in pure SQL and markdown
https://evidence.dev
MIT License
3.49k stars 168 forks source link

Source Variable substitution only works for single occurrence #1867

Open smilingthax opened 1 month ago

smilingthax commented 1 month ago

Steps To Reproduce

(This can also be reproduced using a real sources/ file, but is harder to show).

Import subSourceVariables from https://github.com/evidence-dev/evidence/blob/next/packages/lib/plugin-connector/src/data-sources/sub-source-vars.js into nodejs:

process.env.EVIDENCE_VAR__var_a = 'abc';   // "hack", usually already set in the calling environment
process.env.EVIDENCE_VAR__var_b = 'def';

console.log(subSourceVariables('|${var_a}|${var_b}|'));

Environment

Expected Result

'|abc|def|'

Actual Result

'|abc|${vardef'

Workarounds

Maybe: Use only a single variable?

Better: Fix implementation to not rely on (or: adjust) match indices that become invalid after the first replacement.

smilingthax commented 1 month ago

I believe this whole (buggy) part:

    let output = queryString;
    // This regex is prefixed with a negative lookbehind to disqualify $${var} patterns
    const regex = RegExp(/(?<!\$)\$\{(.+?)\}/, 'g');

    let match;
    while ((match = regex.exec(queryString)) !== null) {
        const fullMatch = match[0]; // e.g. ${variable}
        const varName = match[1]; // e.g. variable
        const start = match.index;
        const end = match[0].length + start;
        if (...replacement exists...) {
            const value = [... obtain replacement...]
            const before = output.substring(0, start);
            const after = output.substring(end);
            output = `${before}${value}${after}`;

can simply be replaced with

    // This regex is prefixed with a negative lookbehind to disqualify $${var} patterns
    const regex = RegExp(/(?<!\$)\$\{(.+?)\}/, 'g');

    let output = queryString.replaceAll(regex, (fullMatch, varName) => {
        return [...obtain replacement or warn/throw];
    });

(in older JS standards, .replace(regex, (...) => ...) would also work; in newer JS, .replaceAll is a bit clearer.)