Bad documentation about boolean value of strings

[CarloCannas]

Currently the documentation states:

String types will evaluate to false if they are empty

To me this means: empty strings (i.e. "") are casted to false in boolean context, which is not the actual behaviour.

A simple VTC showing this:

varnishtest "Truthy values"

server s1 {}

varnish v1 -vcl+backend {
        import std ;

        sub vcl_recv {
                if (!(
                        req.http.testa+req.http.testb && # two unset headers concatenated evaluate to true
                        "" &&                            # an empty string evaluates to true
                        !req.http.test &&                # an unset header evaluates to false
                        req.http.empty                   # an header set to an empty string evaluates to true
                )){
                        return (fail);
                }
                return (synth(200));
        }
} -start

client c1 {
        txreq -hdr "Empty: "
        rxresp
        expect resp.status == 200
} -run

I assume this is the intended behaviour, but the documentation is unclear/wrong.

I think that paragraph should be reworded to make clear that any string in a boolean context evaluate to true, even if empty, and that unset values evaluates to false.

I'm not sure if unset value is the right wording, what I mean with that is headers which have never been set or which have been unset.

[Dridi]

I think we should really keep the current behavior for the if (req.http.empty) case for backwards compatibility reasons and because that allows us currently to tell whether at least one header field is present for a given name.

I'm in favor of a docfix, not against the idea of a VCL 4.2 where empty strings become falsy as long as we have a vmodless syntax to detect set headers.

[nigoroll]

my vote goes to docfix like:

String types will evaluate to false if they are unset (NULL)

[Francois-v3]

IMHO, "String types will evaluate to |false| if they are not set (NULL)" is better. Le 28/11/2018 à 10:19, Nils Goroll a écrit :

my vote goes to docfix like: String types will evaluate to |false| if they are unset (NULL) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2846 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AL_onHn8-CGJxw2euGUKG-ieb7B-cgRjks5uzlUdgaJpZM4Y2ld7>.

-- François Veux - +33633267508

[fgsch]

There are 2 problems here:

• A documentation issue
• A bug in the code such as req.http.nonexistent0 + req.http.nonexistent1 return an empty string and not NULL.

Fixing the former is straight forward. For the latter I believe the diff below should do it but it might be difficult to implement as it might break existing installations:

diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index 0c28ba29e..7b2a09745 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -284,7 +284,7 @@ VRT_String(struct ws *ws, const char *h, const char *p, va_list ap)
 		if (q == vrt_magic_string_end) {
 			va_end(aq);
 			WS_Release(ws, 0);
-			return ("");
+			return (NULL);
 		}
 		do
 			p = va_arg(aq, const char *);
@@ -334,7 +334,7 @@ VRT_CollectString(VRT_CTX, const char *p, ...)
 	va_start(ap, p);
 	b = VRT_String(ctx->ws, NULL, p, ap);
 	va_end(ap);
-	if (b == NULL)
+	if (b == NULL && WS_Overflowed(ctx->ws))
 		VRT_fail(ctx, "Workspace overflow");
 	return (b);
 }

[bsdphk]

Bugwash conclusion:

Docfix.

Our treatment of NULL is similar to how Python deals with unset values (None).

NULL + NULL returning "" is consistent with our "everything can be turned into a string" rule for VCL.