AlinsRan commented on PR #13686:
URL: https://github.com/apache/apisix/pull/13686#issuecomment-5114688528
The full-line skip does fix the reported case, but I'd suggest deciding
whether we want to keep patching at the text level at all before merging this.
Three things I verified by calling master's `resolve_conf_var_in_text()`
directly (`82fcf548f`):
```
key: 1 # ${{UNSET}} -> ERR: can't find environment variable UNSET
# inline comment, not covered here
key: ${{V}} V='secret # x' -> {"key":"secret"}
# value silently truncated
key: ${{V}} V=$'a\nb: 2' -> {"b":2,"key":"a"}
# env value injects a top-level key
```
And one regression this PR would add: a block-scalar content line starting
with `#` is now skipped, so
```yaml
response_example: |
# ${{TITLE}}
```
silently stops being substituted.
The root cause is that pre-parse text substitution (#13078) re-implements
YAML lexing outside lyaml. An alternative that avoids all four: replace each
`${{...}}` with a unique plain-scalar token before parsing (recording var /
default / whether it was directly wrapped in quotes), let lyaml parse —
comments, block scalars and quoting are then handled by the parser — then
resolve the tokens that survived into the parsed tree. "Env var not found" only
fires when a reference actually reaches the config, so commented-out references
never fail by construction, in either comment style.
I prototyped that and ran it against the cases #13078 added plus the new
ones:
```
full-line comment {"key":1} inline comment {"key":1}
missing var ERR (unchanged) unquoted number {"weight":3}
quoted number {"key":"3"} unquoted bool {"flag":true}
default fallback {"key":"fallback"} var in key {"mykey":1}
embedded in string {"key":"pre-3-post"} big int quoted
{"id":"356002209726529540"}
value containing # {"key":"secret # not-a-comment"}
multiline value {"key":"a\nb: 2"} block scalar {"body":"# 3
stays\n"}
```
One intentional behavior change there: env values are always injected as
scalars, so `nodes: ${{NODES_JSON}}` no longer expands into a YAML structure.
That is the same path as the injection case above, so I'd argue for closing it,
but it needs a changelog note.
If the preference is to keep this PR minimal, it should at least become
comment-aware instead of line-aware: find the first `#` that is outside quotes
and preceded by start-of-line or whitespace, substitute only what precedes it,
and track `|`/`>` block-scalar ranges so their content is still substituted.
That covers the inline-comment case and removes the regression, though
truncation and injection would remain.
Ref #13685.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
