v1.5.0 breaking change: invalid value for string type: null

[hurrycaner]

Describe the bug
When i send this request to flipt /api/v1/evaluate:

{"flag_key":"profile","entity_id":"ccf4c0a9-fc44-4478-a790-a01762efff01","context":{"cohort":null}}

I'm getting the following response body as status 400:

{"code":3,"message":"proto: (line 1:102): invalid value for string type: null","details":[]}

This is a bug because this does break the default behavior and was not documented in changelog.

Version Info
version 1.5.0 as docker image.

To Reproduce
The steps are well written in the description

Expected behavior
The response should be status 200, with body similar to this:

{
    "requestId": "728457c6-7dcd-46ff-9779-73b8988422a0",
    "entity_id":"ccf4c0a9-fc44-4478-a790-a01762efff01"
    "requestContext": {
        "cohort":null
    },
    "match": false,
    "flagKey": "profile",
    "segmentKey": "",
    "timestamp": "2022-01-25T17:13:51.337894649Z",
    "value": "",
    "requestDurationMillis": 3.700846
}

• Using postgres

[markphelps]

Thanks @hurrycaner , do you remember which version you were running before 1.5 that passing null was allowed? I'm wondering if this is due to an upgrade in grpc or grpc gateway

[markphelps]

This seems like it may be the underlying issue: golang/protobuf#1361

Looks like protobuf hasn't released a new release with the fix since its has been superseded by https://pkg.go.dev/google.golang.org/protobuf the README.

I'll try upgrading to the new package and add a test to check the fix

[mfbmina]

Hello @markphelps! on 1.4 it works if you pass null

[markphelps]

Thanks @mfbmina ! It seems the regression was introduced in #594 when I upgraded grpc-gateway to v2. Theres an issue on the grpc-gateway project here: grpc-ecosystem/grpc-gateway#2481

It seems like there are two options:

1. revert the upgrades to grpc-gateway back to v1
2. implement a custom marshaller as mentioned in this comment

I'd prefer to go with option 2 if its simple enough, but I'll have to try it later this afternoon

[markphelps]

@hurrycaner if this is blocking you I'd recommend either:

1. downgrading to Flipt 1.4.0 until we can get this fixed, as there were no functional changes introduced in v1.5.0, only dependency upgrades
2. updating your calling code to not pass null as a map value in context, I do understand this is not ideal though

[mfbmina]

I believe that option 3 will break https://semver.org/.

Another option could be releasing two new versions: 1.6.0 and 2.0.0

• 1.6 rollbacks the lib and keeps the old behavior
• 2.0 is technically the same version as 1.5

What do you think?

[markphelps]

@mfbmina while you are correct that 1.5.0 did technically break semver because it introduced a breaking change to the public API it did so unintentionally so I'd prefer not to increment the major version once the fix is in.

Semver actually has a section in the FAQ discussing what to do this in this situation:

As soon as you realize that you’ve broken the Semantic Versioning spec, fix the problem and release a new minor version that corrects the problem and restores backwards compatibility. Even under this circumstance, it is unacceptable to modify versioned releases. If it’s appropriate, document the offending version and inform your users of the problem so that they are aware of the offending version.

Also I don't think passing a null literal as JSON value for the context type map is a very common case, so I feel as though in this case its ok to simply note in the README and release of 1.5.0 that 1.5.0 introduced this breaking change, and then release a 1.5.1 once the fix is in

[mfbmina]

Sounds good enough!

[mfbmina]

Thanks @markphelps

[markphelps]

I'm going to open an issue to talk about Flipt v2 API, because there are some breaking changes that have been requested in the past, I just gotta dig them up and link them.

There's also some breaking changes for the configuration file that I'd like to make as well, such as those likely required for #633 and #576

Then we could make the proper fix (to not handle nulls) in v2 since according to grpc-ecosystem/grpc-gateway#2481 they arent valid in proto and reference this issue and update the open API docs as well.

[markphelps]

@hurrycaner @mfbmina This should now be fixed in v1.5.1

See here for changes

Thanks again for the bug report!