[BUG] Missing value with ResolveBoolean when variant is off

[L3o-pold]

Observed behavior

Using flagd:v0.6.2 the /schema.v1.Service/ResolveBoolean route respond without a value field. The PHP SDK then think it's an error.

Expected Behavior

I guess value field is mandatory.

Steps to reproduce

Use this kind of config file.

{
    "flags": {
        "test": {
            "state": "ENABLED",
            "variants": {
                "on": true,
                "off": false
            },
            "defaultVariant": "on",
            "targeting": {
                "if": [
                    {
                        "$ref": "ipWithoutTest"
                    },
                    "off",
                    "on"
                ]
            }
        }
    },
    "$evaluators": {
        "ipWithoutTest": {
            "in": [
                {
                    "var": [
                        "ip"
                    ]
                },
                [
                    "10.0.0.1",
                    "10.0.0.2"
                ]
            ]
        }
    }
}

Then query with an IP present in the evaluator:

curl --location 'http://localhost:8003/schema.v1.Service/ResolveBoolean' \
--header 'Content-Type: application/json' \
--data '{
    "flagKey": "test",
    "context": {
        "ip": "10.0.0.1"
    }
}'

In the response the value field is missing

{
    "reason": "TARGETING_MATCH",
    "variant": "off",
    "metadata": {}
}

If you do the same query with an IP that's not matching the evaluator rule, the value attribute is present.

{
    "value": true,
    "reason": "TARGETING_MATCH",
    "variant": "on",
    "metadata": {}
}

[beeme1mr]

Hey @L3o-pold, sorry you ran into this issue. This is the default behavior of proto3 JSON encoded output. We've documented the behavior here.

It looks like it may be possible to override this default behavior.

https://protobuf.dev/programming-guides/proto3/#json-options

I think this may be worth exploring. FYI @toddbaert @Kavindu-Dodan

[L3o-pold]

@beeme1mr if it's the normal behavior from flagd, maybe we need to close the issue here and create a new one on the PHP sdk repository as it's seen as an error.

[toddbaert]

@L3o-pold as of now, it's expected. However, with the doc @beeme1mr points out it may be worth revisiting, so I will keep this issue open for a bit while I investigate.

[toddbaert]

It looks like it may be possible to override this default behavior.

https://protobuf.dev/programming-guides/proto3/#json-options

I can't find any means of configuring this with the code generation we're using, or in the generated code itself, though it seems to be implemented in the underlying go protobuf libraries.

Maybe the documentation is incomplete or I'm looking in the wrong place. I'm going to look into it a bit more tomorrow.

[beeme1mr]

This may help @toddbaert

https://github.com/connectrpc/connect-go/pull/500

It looks like we would either have to write a small wrapper around protoJSONCodec or use the library listed in the comments.

[toddbaert]

@beeme1mr thanks. I looked into the lib mentioned there, and it seems a bit too unpopular to merit inclusion, IMO. I'd rather not depend on it.

I used the same concepts to build a little json-marshaller inspired by it, and it seems to be a means of solving the problem. I have lots of remaining questions but we can probably go this route.

[toddbaert]

I've created a fix for this, see here. Thanks for pointing me in the right direction @beeme1mr

[toddbaert]

This will be released with the merge of https://github.com/open-feature/flagd/pull/780

[toddbaert]

@beeme1mr I'm reopening this issue because though we "fixed" this behavior in json, I believe there's nothing to be done about it in actual protobuf messages.

This behavior is fundamental to proto3: https://protobuf.dev/programming-guides/proto3/#default

Note that for scalar message fields, once a message is parsed there’s no way of telling whether a field was explicitly set to the default value (for example whether a boolean was set to false) or just not set at all: you should bear this in mind when defining your message types. For example, don’t have a boolean that switches on some behavior when set to false if you don’t want that behavior to also happen by default. Also note that if a scalar message field is set to its default, the value will not be serialized on the wire.

This means that flagd providers that speak gRPC/protobuf (and not HTTP/Json) will still need to interpret missing values as zero-values ("", 0, false, etc). We may need to restore some of the doc I removed.

Some language implementations will handle this automatically for us when they deserialize the protobuf (Java) while some may not (JS).

[toddbaert]

Re-closing this since it only concerns JSON.