Repository navigation
fix: JSON-escape values templated into a component's vega_spec - #10042
Conversation
| props := canvas.CoerceScalarParams(spec.RendererProperties.AsMap(), spec.Params, effectiveArgs) | ||
|
|
||
| // A custom chart's vega_spec is a JSON document held in a string, so it is set aside and | ||
| // resolved per JSON string below; resolving it as one string would splice values in unescaped. |
There was a problem hiding this comment.
Is there a path here where we could escape the values already in TemplateData while calling ResolveTemplate? We could avoid unmarshal => marshal altogether.
There was a problem hiding this comment.
Done in 7991e9f: the vega_spec is now resolved as one template with a copy of the template data in which every string in .params, .fields, .env and .user is JSON-escaped (canvas.EscapeJSONStrings). No unmarshal or marshal, and template scope across JSON strings is unchanged. The one trade-off is that a string value spliced in as raw JSON (outside a JSON string) is escaped too; I noted it in the description.
|
|
||
| // UseNumber keeps numeric literals as written, so re-encoding doesn't alter their precision or notation. | ||
| dec := json.NewDecoder(strings.NewReader(vegaSpec)) | ||
| dec.UseNumber() |
There was a problem hiding this comment.
Does this really help? InjectVegaParams doesnt do the same so precision is lost anyway no?
There was a problem hiding this comment.
Agreed, it only helped the no-scalar-param case. Gone with the rewrite: the spec is no longer decoded here at all.
| return string(out), nil | ||
| } | ||
|
|
||
| func resolveJSONStrings(val any, resolve func(string) (string, error)) (any, error) { |
There was a problem hiding this comment.
nit: reuse ResolveTemplateRecursively
There was a problem hiding this comment.
Moot with the rewrite: there is no JSON walk any more, and runtime/canvas cannot import the parser anyway since runtime/parser imports runtime/canvas.
73308af to
7991e9f
Compare
ResolveComponent resolved a custom chart's vega_spec with the plain template data, so a field display name or string param containing a quote or backslash was spliced into the JSON unescaped. An ejected chart bound to a measure with display_name 'Revenue "net"' then failed in InjectVegaParams (or, without scalar params, in the renderer as "Invalid Vega-Lite spec"). Resolve the vega_spec with a copy of the template data in which every string (.params, .fields, .env, .user) is JSON-escaped, so substituted values are valid inside the JSON strings they land in. The spec is still resolved as a single template, so template scope across JSON strings is unchanged, and the scalar params are injected from the unescaped args as before.
7991e9f to
1df318b
Compare
AdityaHegde
left a comment
AdityaHegde
left a comment
There was a problem hiding this comment.
Since we now pre-escape some edge cases like
{{ if eq .params.label "a \"b\"" }}, dont match anymore.{{ len .params.label }}will return escaped length.
We could perhaps document this so users are not thrown off guard?
|
Sure, will add docs in #10029 PR. |
Since #10042, a component's vega_spec is resolved with JSON-escaped template data. Say so where vega_spec is documented: the Templating section of the Vega-Lite page, the vega_spec schema description (and the generated component reference), and the developer agent's Vega-Lite rules, which now tell it to keep every placeholder inside a JSON string.
2 participants
ResolveComponentresolved a custom chart'svega_specwith the plain template data, so a field display name or string param containing"or\was spliced into the JSON unescaped. An ejected chart bound to a measure withdisplay_name: 'Revenue "net"'then failed inInjectVegaParamswithInvalidArgument, or in the renderer with "Invalid Vega-Lite spec" when the component had no scalar params.vega_specwith a copy of the template data in which every string in.params,.fields,.envand.useris JSON-escaped (canvas.EscapeJSONStrings), so substituted values are valid inside the JSON strings they land in.{"a": "b"}, is now escaped too. Values without quotes or backslashes are unaffected.Steps to reproduce:
customComponentsinrill.yaml.display_name: 'Revenue "net"'.Checklist: