falco
falco copied to clipboard
ysugimoto
STRING/IP not set expansion
Rebase of the ProcessExpression based not_set expansion solution onto the #248 branch.
Setting this as a draft because I had to make a change to the LenientString to make the changes needed for the expression handling work correctly. With that change it largely removes half of the utility of the type which reinforces my feeling that the new type really isn't needed.
The reason for making LenientString type is that I considered that we should keep the notset value in whose steps - it's like a state because I don't know how the notset string is treated at some statements.
Your implementation looks neat but my concern is whether it's okay to determine whether the ident value contains (null) string or not on the single statement. does this cover the multiple statements for treating notset string properly?
Of course, I understand falco is a different approach for the VCL against Fastly (on the fly vs AST-based) so it makes it difficult to reproduce the VCL behavior.
My main concern with the approach of trying to put the (null) handling logic into the type is the fact that the behavior of various null expansion cases depends on the context of the statement in which an expression is being parsed.
For example in the case of string concatenation. Not set values will be expanded to (null) or an empty string depending on several factors.
- Is the expression result assigned to a header? - expand to
(null) - Is the expression result assigned to a string local variable? - handle as empty string
- Is the expression result passed to a log statement? - expand to
(null)
All these different cases require context about where the string concatenation is occurring. It seems to me that the details of handling when to insert (null) vs "" requires enough logic in the expression handling that is makes more sense for it to be internal to that code vs part of the string type that is used everywhere.
does this cover the multiple statements for treating notset string properly?
I don't follow what you mean here, could you please elaborate?
It looks like a NotSet state is determined for each context independently.
Once some HTTP Header value is set with notset-like string as "(null)", the HTTP Header value is treated as string that has
the "(null)" string value, it could be not notset status.
My concern is... NotSet string should be affected following statement and do we need to keep the NotSet state the following statements, for example:
declare local var.S STRING;
set req.http.Foo = var.S;
log req.http.Foo; // => "(null)" string (1)
set req.http.Foo = req.http.Foo var.S;
log req.http.Foo; // => "(null)" string, not "(null)(null)" (2)
Fiddle is here https://fiddle.fastly.dev/fiddle/ccf10a57 The expansion on an expression is so good approach but does this approach cover the above case, particularly (2)?
Ah! You found the one case I forgot to include a test for, nice. The concatenation of two unset values yields an unset value regardless of the target of the assignment.
The first assignment leaves the foo header not_set since assigning a not_set value to a header results in the header being not_set. So the second assignment is between an unset header and an unset string local variable.
Your example is functionally equivalent to:
declare local var.s STRING;
set req.http.foo = req.http.bar var.s;
Fixed this case and added a test for it in my validation cases. https://github.com/richardmarshall/falco-validation-tests/blob/main/not_set/main.test.vcl#L434
Okay, If your implementation covers it, my concern will be solved, thanks.