asdf2014 commented on PR #13686: URL: https://github.com/apache/apisix/pull/13686#issuecomment-5119194280
Agreed on the root cause: pre-parse text substitution re-implements YAML lexing outside lyaml, and the tokenize-then-parse-then-resolve approach is the right end state. It fixes all four cases by construction, and your prototype results (especially `value containing #` and the multiline injection) make a strong case. Closing the structural-expansion path also seems right to me, since it is the same injection surface, and a changelog entry covers it. On the middle option (comment-aware substitution): I would rather not go there. Tracking quotes plus `|`/`>` ranges at the text level is halfway to a YAML lexer, with its own corner cases (multi-line quoted strings, `#` inside flow collections, nested block-scalar indentation), while truncation and injection still remain. If we want more than the minimal fix, the token approach is the better place to spend that complexity. So my suggestion for sequencing: keep this PR as the minimal stopgap for the 3.17 regression (#13685 blocks startup today, and this change is byte-transparent for comment-free input), and land your tokenization as the follow-up that replaces this code path entirely. The block-scalar direction change here is documented in the code comment and hits nothing in-tree, and it disappears together with the rest of the text-level heuristic once tokenization lands. If the maintainers would rather go straight to the token approach in one step, I am also happy to close this in favor of your prototype, whichever you prefer. -- 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]
