Update null feature parsing logic

[masonj5n]

About the changes

I was trying out Unleashed for the first time (product is awesome!) but noticed features would never become enabled in the Rust client. I noticed if I only had features with descriptions things did work, and tracked it down to the poller failing to parse the features from json. Not relevant to the Rust sdk but it does seem like if you create a new feature and don't give a description, the field is null in the json response, but if you create it with a description but then erase the description and save it, it returns "" instead of null.

In any case, the description field on a feature can be null - when it is null it fails to parse since it's represented by a String in the Rust representation.

The obvious way to fix this would be just changing description: String to description: Option<String>, but it does seem like it was intentional since there are other optional fields and it has a #[serde(default)] attribute. Maybe someone has some more context on that?

The default serde attribute isn't enough in this case because it doesn't apply when the field is present, but set to null, it only applies if the field isn't given at all.

Assuming it was intentional to not make the description optional, this parser should turn an incoming description: null into description: "".

I added a test that fails before this change but passes afterward.

Important files

Discussion points

If it wasn't intentional to make the description not optional we can go that route instead and just make it optional. I think there's some more work to do there since there is other code that assumes the field will be String and not Option<String>, but happy to do that work if that's the route we want to go.

[thomasheartman]

This looks and sounds (and smells?) like a sensible change to me! I'll have a look later on today or this week 😄 Thanks for your contribution 🙏🏼

[masonj5n]

Hey, thanks for the quick response!

Glad to hear we don't need to go that custom deserializer route! I updated the PR to make the field optional instead of removing it completely - the reasoning is just that if the strict feature is enabled, the parsing will always fail since description is always present in the response from the api, iiuc.

I confirmed that would be a problem by deleting the field and then running cargo test --features strict, which started failing.

If we want to remove the field completely it seems like it might be best to also remove the strict feature from the crate, since if we delete the field that crate feature will always break the parsing? Or maybe just have that feature not affect the Feature struct?

If the change to optional works I think this should be g2g, happy to remove the field entirely and remove the strict feature or remove the strict parsing attribute from the struct too.

[thomasheartman]

@masonj5n Ah, sorry; looks like clippy found a couple places where you'd forgotten to wrap the name in an Option in benches/is_enabled.rs. Would you mind going to clean up?

Error:    --> benches/is_enabled.rs:175:26
    |
175 |             description: name.clone(),
    |                          ^^^^^^^^^^^^ expected `Option<String>`, found `String`
    |
    = note: expected enum `std::option::Option<std::string::String>`
             found struct `std::string::String`
help: try wrapping the expression in `Some`
    |
175 |             description: Some(name.clone()),
    |                          +++++            +

error[E0308]: mismatched types
Error:    --> benches/is_enabled.rs:189:26
    |
189 |             description: name.clone(),
    |                          ^^^^^^^^^^^^ expected `Option<String>`, found `String`
    |
    = note: expected enum `std::option::Option<std::string::String>`
             found struct `std::string::String`
help: try wrapping the expression in `Some`
    |
189 |             description: Some(name.clone()),
    |                          +++++            +

For more information about this error, try `rustc --explain E0308`.
error: could not compile `unleash-api-client` (bench "is_enabled") due to 2 previous errors
Error: warning: build failed, waiting for other jobs to finish...
Error: The process '/home/runner/.cargo/bin/cargo' failed with exit code 101

[masonj5n]

Whoops, sorry about that! Should be good now

[thomasheartman]

@masonj5n Thanks a lot for your contribution! 🙏🏼 I took the liberty of rebasing your changes into a single commit and adding a little more context in the commit message. I'll go ahead and merge this once the CI jobs have run. Will sort out a release shortly too 😄