Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify Json Decoder to handle missing/optional Json fields if mapped to Option #12794

Closed
quux00 opened this Issue Mar 9, 2014 · 18 comments

Comments

Projects
None yet
10 participants
@quux00
Copy link

quux00 commented Mar 9, 2014

Currently, the Json Decoder fails if the JSON it is decoding lacks a field in the Decodable struct it is mapping onto.

I propose that if a struct field maps to an Option, the Json Decoder would handle missing fields by mapping None to the field.

Here's a code example:

extern crate serialize;

#[deriving(Decodable)]
pub struct LogEntry {
    index: u64,
    term: u64,
    command_name: ~str,
    command: Option<~str>,  // optional in the JSON
}

#[cfg(test)]
mod test {
    use serialize::{json, Decodable};

    #[test]
    fn test_json_decode_of_LogEntry() {
        let jstr = ~r##"{"index": 200, "term": 4, "command_name": "foo"}"##;
        let jobj = json::from_str(jstr);

        let mut decoder = json::Decoder::new(jobj.unwrap());
        let decoded_obj: super::LogEntry = Decodable::decode(&mut decoder);  // fails

        assert_eq!(None, decoded_obj.command);
    }
}

This fails at runtime with the error message:

---- test::test_json_decode_of_LogEntry stdout ----
    task 'test::test_json_decode_of_LogEntry' failed at 'JSON decode error: missing required 'command' field in object: {}', /home/quux00/rustlang/rust/src/libserialize/json.rs:1128

I discussed this in the #rust IRC channel and cmr said this sounded reasonable when optional fields map to Option<> fields and suggested I file it here as a enhancement request.

@ZeroOne3010

This comment has been minimized.

Copy link

ZeroOne3010 commented Apr 17, 2014

Hi @quux00! Did you come up with any good workaround for this issue?

@quux00

This comment has been minimized.

Copy link
Author

quux00 commented Apr 17, 2014

The workaround was to have the "null" field be empty string instead. Not very satisfactory. I ended up not needing the Option<> field in my struct so haven't worked on this issue. I just tried the latest version of rustc to see how it works now, but got caught in a endless cycle of compiler errors that I didn't have enough motivation to wade through.

@ZeroOne3010

This comment has been minimized.

Copy link

ZeroOne3010 commented Apr 17, 2014

So you were also in control of the JSON format that you were trying to decode? I unluckily am not, so I need to find some other way.

@tomjakubowski

This comment has been minimized.

Copy link
Contributor

tomjakubowski commented Apr 17, 2014

If this is approved as an enhancement I'd be interested in tackling this.

@compmstr

This comment has been minimized.

Copy link

compmstr commented Apr 17, 2014

I'm currently working on a fix for this. I have a simple one that only works for Json, but I'm working on updating the Deccodable expansion to provide an optional field, so it will work with any type where an optional value will work. I'm stuck on figuring out what type an object is during deriving(Decodable) expansion, however.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Apr 17, 2014

So, the problem comes from Decoder not knowing that a value is optional until the field name is read:

// ...
// tries to read field before read_option
self.command = try!(d.read_struct_field("command", 3u, |d| Decodable::decode(d)));
// ..

A special case would likely need to be added in the deriving code to check if the value should be an option, and not Err if not found.

Currently, you can get around this by just not deriving(Decodable), since you can't use the default impl. For the LogEntry example, it could look like this:

impl<D:Decoder<E>, E> Decodable<D, E> for LogEntry {
    fn decode(d: &mut D) -> Result<LogEntry, E> {
        d.read_struct("LogEntry", 4u, |d|
            Ok(LogEntry {
                index: try!(d.read_struct_field("index", 0u, |d| Decodable::decode(d))),
                term: try!(d.read_struct_field("term", 1u, |d| Decodable::decode(d))),
                command_name: try!(d.read_struct_field("command_name", 2u, |d| Decodable::decode(d))),
                command: match d.read_struct_field("command", 3u, |d| Decodable::decode(d))) {
                    Ok(opt) => opt,
                    Err(_) => None
                }
            })
        )
    }
}
@tomjakubowski

This comment has been minimized.

Copy link
Contributor

tomjakubowski commented Apr 17, 2014

@quux00 Not sure if you're already aware, but an alternative to using the empty string to represent an "empty" string field would be use a null value for that key in the JSON, which will deserialize to None:

{"index": 200, "term": 4, "command_name": "foo", "command": null}

deserializes to LogEntry { index: 200, term: 4, command_name: foo, command: None }. The problem you reported is still there for missing keys though.

Given that null can be used to deserialize a field as None, would it be reasonable to provide two Decoder structs in serialize::json: one which defaults any missing Option fields as None, and a stricter one which has the current behavior of erroring when any field is missing?

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Apr 18, 2014

@tomjakubowski it'd be quite difficult to do that. As I said, the call to read_struct_field happens before read_option, so the JsonDecoder isn't able to know that the struct field should be optional.

@tomjakubowski

This comment has been minimized.

Copy link
Contributor

tomjakubowski commented Apr 18, 2014

Didn't see your comment while I was writing mine, makes sense.

@compmstr

This comment has been minimized.

Copy link

compmstr commented Apr 18, 2014

@tomjakubowski @seanmonstar The quick way I had set it up for json only was to try read_option if read_struct_field failed, and if read_option failed, to only then throw the missing field error.

@compmstr

This comment has been minimized.

Copy link

compmstr commented Apr 18, 2014

Sorry, I mistyped, it puts a null on the stack, then tries read_option, so that if it was an optional field, it will read null/None, then if that fails it wasn't an optional field.

@pzol

This comment has been minimized.

Copy link
Contributor

pzol commented Aug 2, 2014

Anybody working on this??

@bguiz

This comment has been minimized.

Copy link
Contributor

bguiz commented Aug 6, 2014

+1 for Option<T> to represent fields that may not be present; I would like to see this implemented.

Concerns for symmetry though:

Let's say that you have a field whose type is Option<T>, and the object, prior to serialisation, contains None for that value, what should the serialised output look like:

  • Should it contain that key with a value of null, or should that key not be present?
  • Should we have a mechanism for being able to specify default values?
@pzol

This comment has been minimized.

Copy link
Contributor

pzol commented Aug 10, 2014

How about passing an object with defaults and passing it to decode?

#[deriving(Decodable)]
struct Category {
    min: uint,
    max: uint
}

//...
let mut category = Category { min: 0, max: 5 };
let category = Decodable::decode(&mut decoder, &mut category);
@bguiz

This comment has been minimized.

Copy link
Contributor

bguiz commented Aug 10, 2014

How about passing an object with defaults and passing it to decode?

That sounds good, although I would still prefer being able to specify the defaults when declaring the struct.

@erickt

This comment has been minimized.

Copy link
Contributor

erickt commented Sep 1, 2014

@pzol: I'm working on this as part of my serialization rewrite rust-lang/rfcs#22.

@mitchmindtree

This comment has been minimized.

Copy link
Contributor

mitchmindtree commented Sep 3, 2014

I've looked through a few issues now and I'm struggling to find where this has previously been mentioned:

Is it possible to require the std::default::Default trait for all types that implement Decodable to solve the missing fields issue? I'm sure there are reasons something like this has been avoided, I just haven't run across them yet and I'd love to hear them!

My recent use case is within my audio synthesis instrument library - development currently involves lots of back and forth between testing and adding/removing fields. Each time I add or remove fields within the data structure my library loading starts to fail and I have to either discard the library and start designing the instruments again from scratch or manually add/remove fields to/from the .json file. If I were able to specify default values instead of having the loading fail, it would make the whole process much smoother.

I understand that this may not be an issue for other users of Decodable - Perhaps it would be better to have a separate trait (i.e. PartiallyDecodable) that uses defaults in the decode method where Decodable would normally fail?

@treeman

This comment has been minimized.

Copy link
Contributor

treeman commented Sep 3, 2014

@erickt I did not see your comment before writing my PR for this issue. RFC seems useful.

@mitchmindtree I do think some manner of defaults for fields should be there, but I'm not certain how best to approach it. It would require a slightly larger rewrite of Decodable whatever way we want to do it. Maybe add these comments to @erickt's rust-lang/rfcs#22?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.