Closed Andrew-Morozko closed 2 weeks ago
One question regarding ref
s:
document "hello" {
content text "base" {
vars {
a = "original"
b = query_jq(".vars.a")
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref" {
base = document.hello.content.text.base
vars {
a = "redefined"
}
}
}
I assume that here the base block outputs "a": "original", "b": "original"
and the ref block outputs "a": "redefined", "b": "original"
(and not "a": "redefined", "b": "redefined"
). Otherwise reasoning about values of vars
becomes too difficult and non-local.
In other words, shadowing happens after evaluation, not before. Am I correct? @traut
Another complicated situation:
document "hello" {
vars {
a = "original"
}
content text "base" {
vars {
b = query_jq(".vars.a")
a = "redefined"
}
value = "{{toPrettyJson .vars}}"
}
}
This should output "a": "redefined", "b": "original"
, I guess? So, the redefinitions happen in order of variable declaration.
If so then
document "hello" {
content text "base" {
vars {
a = {
b = "val"
c = query_jq(".vars.a.b")
}
}
value = "{{toPrettyJson .vars}}"
}
}
must output {"a": {"b": "val", "c": null}}
, since the a
is not set when query_jq
is evaluated
@Andrew-Morozko made the issue for env vars -- https://github.com/blackstork-io/fabric/issues/191
I assume that here the base block outputs "a": "original", "b": "original" and the ref block outputs "a": "redefined", "b": "original" (and not "a": "redefined", "b": "redefined"). Otherwise reasoning about values of vars becomes too difficult and non-local. In other words, shadowing happens after evaluation, not before.
wouldn't it be easier to shadow before evaluation, though? We collapse the var definitions first, and evaluate after. In your example, I would kind of expect "a": "redefined", "b": "redefined"
. It would also follow the theme that the referenced block is not evaluated but the referencing one is.
I might be off here, but I can't think of when this breaks the expected output. What do you think?
damn, those edge cases 😅
This should output "a": "redefined", "b": "original", I guess? So, the redefinitions happen in order of variable declaration.
I think you're right.
must output {"a": {"b": "val", "c": null}}, since the a is not set when query_jq is evaluated
that's totally reasonable!
wouldn't it be easier to shadow before evaluation, though?
It is kinda easier, but in the end it's just a question of what is more expected by the user (and easier to describe in the docs 😉). I also think that "a": "redefined", "b": "redefined"
better matches the established ref
block mechanics, but I also imagined debugging this example:
document "hello" {
content text "base" {
vars {
a = ["original"]
b = query_jq(".vars.a[0]")
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref" {
base = document.hello.content.text.base
vars {
a = {
b = "redefined"
}
}
}
}
Which would produce this error:
Error: Failed to run the query
on zzz.fabric line 5, in document "hello":
5: b = query_jq(".vars.a[0]")
expected an array but got: object ({"b":"redefined"})
And the user would look at lines 4 and 5 and not understand where that object came from.
But perhaps the solution is including a note "this error was triggerd through ref at ...", not special-casing var handling for refs.
@Andrew-Morozko let's go with "a": "redefined", "b": "redefined"
, so collapse/shadow the vars and evaluate after.
You are right, making it clear is very important. I think it's straightforward to explain this way.
But perhaps the solution is including a note "this error was triggerd through ref at ...", not special-casing var handling for refs.
Yeah, if the error is in the ref block, we can use different wording or provide more context!
One more tricky situation:
Right now ref vars
are executed after the base vars
, except in cases where var in ref shadows the one in base. In this case, it is executed at the same time as var in base would've been executed, if not for override.
This leads to yet another instance of quirky behavior:
document "hello" {
content text "base" {
vars {
a = "original"
b = "unique to base"
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref-if-after (this is the current behavior)" {
base = document.hello.content.text.base
vars {
c = "unique to ref"
q_b = query_jq(".vars.b") // works as expected
a = query_jq(".vars.c") // doesn't have access to c, because "a" overrides the variable in the "base", and therefore is executed before c is set
}
}
// If we evaluate vars of the ref before vars of the base another issue occurs:
content ref "ref-if-before" {
base = document.hello.content.text.base
vars {
c = "unique to ref"
q_b = query_jq(".vars.b") // doesn't have access to b, vars in ref are evaluated before vars in base
a = query_jq(".vars.c") // works as expected
}
}
}
If we're staying with the "a": "redefined", "b": "redefined"
(base vars are overridden before evaluation), then this would seem to be an intractable problem. Current behavior seems to be the optimal solution in that case, it just needs to be documented.
Right now ref vars are executed after the base vars, except in cases where var in ref shadows the one in base.
Why split it into two execution steps? To avoid re-executing vars in the base block if there are multiple ref
blocks?
content ref "ref-if-after (this is the current behavior)" { base = document.hello.content.text.base vars { c = "unique to ref" q_b = query_jq(".vars.b") // works as expected a = query_jq(".vars.c") // doesn't have access to c, because "a" overrides the variable in the "base", and therefore is executed before c is set } }
That is caused by the split into two steps. Merging the vars and executing after would resolve this issue: a
would de redefined and would return unique to ref
, if I understand correctly
// If we evaluate vars of the ref before vars of the base another issue occurs:
Yeah, same problem. It feels like the vars need to be merged before any evaluation happens. Otherwise, we have this implicit order of evaluation between blocks that is a bit confusing.
Why split it into two execution steps?
No, It's all a single execution step, I'm talking about the variable evaluation order (the order in which the variable values are set in the context map). Gojq works on plugin.Data
, but variables are returned as cty.Value
after evaluating the corresponding fcl
expression. So we need to go over the list of cty.Value
s in some order, turn them into plugin.Data
and set the corresponding key on (data context).vars.
In order to get "a": "redefined", "b": "redefined"
in this example, variables should be evaluated in this order (shown in comments)
document "hello" {
content text "base" {
vars {
a = "original" // not evaluated for ref
b = query_jq(".vars.a") // 2
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref" {
base = document.hello.content.text.base
vars {
a = "redefined" // 1
}
}
}
Applying this evaluation order to my latest example:
document "hello" {
content text "base" {
vars {
a = "original" // never evaluated for ref
b = "unique to base" // 2
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref-if-after (this is the current behavior)" {
base = document.hello.content.text.base
vars {
c = "unique to ref" // 3
q_b = query_jq(".vars.b") // 4
a = query_jq(".vars.c") // 1, because it replaces the "a" from base
}
}
}
We can, of course, not change the evaluation order:
document "hello" {
content text "base" {
vars {
a = "original" // never evaluated for ref
b = "unique to base" // 1
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref-if-after (this is the current behavior)" {
base = document.hello.content.text.base
vars {
c = "unique to ref" // 2
q_b = query_jq(".vars.b") // 3
a = query_jq(".vars.c") // 4
}
}
}
but then this breaks the first example:
document "hello" {
content text "base" {
vars {
a = "original" // not evaluated for ref
b = query_jq(".vars.a") // 1, and would fail, a is not defined yet
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref" {
base = document.hello.content.text.base
vars {
a = "redefined" // 2
}
}
}
Same analysis for the second case (ref-if-before):
document "hello" {
content text "base" {
vars {
a = "original" // never evaluated for ref
b = "unique to base" // 4
}
value = "{{toPrettyJson .vars}}"
}
content ref "ref-if-before" {
base = document.hello.content.text.base
vars {
c = "unique to ref" // 1
q_b = query_jq(".vars.b") // 2, and fails, b is not defined yet
a = query_jq(".vars.c") // 3
}
}
}
aha, understood, thank you for breaking it down!
I think the order in the first snippet makes the most sense, at least for now. Is that the one you are leaning toward, too?
Is that the one you are leaning toward, too?
Yep, that's the most logical one
@Andrew-Morozko if rows_var
is implemented by Fabric, outside the table
content provider, it makes sense to rename it into a more generic collection_var
or similar, so that we can reuse it for other providers
Finally implemented row vars. Here's the implementation, including changes discussed on slack.
document "test" {
vars {
a = 1
b = query_jq(".vars.a + 1")
xxx = "xxx"
}
content table {
rows_var = query_jq("[10, 20, 10*(.vars.b + 1)]")
columns = [
{ header = "{{ .block.col_index }} Value", value = "{{ .block.row }}"},
{ header = "{{ .block.col_index }} Index", value = "{{ .block.row_index }}"},
{ header = "{{ .block.col_index }} ValueFromContext {{ .vars.xxx }}", value = "{{ .vars.xxx }}"},
{ header = "{{ .block.col_index }} StaticValue", value = "foo"},
]
}
}
Result:
|1 Value|2 Index|3 ValueFromContext xxx|4 StaticValue|
|---|---|---|---|
|10|1|xxx|foo|
|20|2|xxx|foo|
|30|3|xxx|foo|
Resolves #166 #191 #169
query_jq
function, available only withinvars
Works even if called in arbitrarily nested FCL lists/mapsvars
block in the order of their definition, access to other vars from parent scopes andref base
blocks. If a variable has the same name as in base - it takes its place in evaluation order (akin to hoisting)vars
in documentsvars
in sections and content.env
filesmyshell = "I use ${env.SHELL} shell"
ref
blocks[x] remove
from_env_variable
funcLeftover tasks (to be dealt with in another PR)
query
and.query_result
removal (right now they are reimplemented with the new query engine and are executed aftervars
evaluation)[ ]
data.inline
removalNew tools
pkg/encapsulator
: a helper for casting back and forth between native go types andcty.CapsuleValue
s in a typesafe mannerplugin.DataPath
for setting/getting values fromplugin.Data
structs via a path (index operations on nested maps/lists with human-readable errors)utils.EvalContextByVar
andutils.EvalContextByFunc
- get the eval context that defines a certain variable or functionuitls.MapMap
anduitls.MapMapErr
: helpers for mapping a function over map values.utils.OnceVal
: same assync.OnceVal
, except natively understands diagnostic semantics. If the first run returns an error - subsequent runs returnRepeatedError
Refactoring to support row_vars
Decided to do it before merging in vars, because some changes to the vars evaluation are needed
Now there is a
definitions.DataCtxEvalNeeded
interface. All values that need dataCtx to be evaluated (such asjq_query
androws_var
) implement it. This deferred evaluation occurs when the cty.Value is converted into another format:jq_query
in var blocks this happens when the variable is evaluated and converted toplugin.Data
rows_var
this will happen when the value ofrows_var
is converted for grpc transmission.In order to prevent duplication of the tricky evaluation/encapsulation logic there now exists
pkg/ctyencoder
: a generic transformer from cty to user-chosen type. For us, it'splugin.Data
andpluginapiv1.CtyValue
(the grpc type).pkg/ctyencoder
provides better error reporting: errors in nested cty objects now come with paths (sequence of indexing operations). To implement thispkg/diagnostics
has been refactored to allow easily addingExtra
values to diagnostics.diagnostics.PrintDiags
uses this extra information to improve diagnostics before they are printed. Circular ref detection diagnostics no longer work by writing the traceback as the function returns, now they modify the corresponding Extra, and the printer is the one responsible for outputting the traceback.Support for passing around
plugin.Data
inside of cty type system and even outside of it (via grpc or with plugin.Data encoding) Simplifiedcty
anddata
protobuf types, now there are fewer nested one-field structs, fewer messages overall, and less code.TODO:
plugin.Data
in content provider args.eval/dataquery/deferred_plugin_data.go
is the WIP type forrows_var
encodeCtyValue
with an evaluator capable of providing dataCtx and ctx for deferred evaluation codeplugin/pluginapi/v1/cty_value_encoder.go