Expose iterators for JSON collections with uniform data types

[DarkFenX]

This suggestion relies on having errors classified between recoverable and non-recoverable, as well as having handling of recoverable errors in the lib.

---

When you have a collection (array, object) where values are of consistent type, it would be convenient to have an iterator interface over those. In my experience it is a relatively typical use-case for JSON reading/writing.

To demonstrate what I mean, this is a simple iterator over arrays using the low-level API I've built for my project :

use struson::reader::{JsonReader, JsonStreamReader};

use crate::phb::parsing::ReadParseFailReason;

pub(in crate::phb) struct ArrayIter<T, R>
where
    T: serde::de::DeserializeOwned,
    R: std::io::Read,
{
    reader: JsonStreamReader<R>,
    opened: bool,
    closed: bool,
    phantom: std::marker::PhantomData<T>,
}
impl<T, R> ArrayIter<T, R>
where
    T: serde::de::DeserializeOwned,
    R: std::io::Read,
{
    pub(in crate::phb) fn new(reader: R) -> Self {
        Self {
            reader: JsonStreamReader::new(reader),
            opened: false,
            closed: false,
            phantom: Default::default(),
        }
    }
}
impl<T, R> Iterator for ArrayIter<T, R>
where
    T: serde::de::DeserializeOwned,
    R: std::io::Read,
{
    type Item = Result<T, ReadParseFailReason>;

    fn next(&mut self) -> Option<Self::Item> {
        if self.closed {
            return None;
        }
        if !self.opened {
            if let Err(error) = self.reader.begin_array() {
                return Some(Err(error.into()));
            }
            self.opened = true;
        }
        loop {
            match self.reader.has_next() {
                Ok(has_next) => match has_next {
                    true => match self.reader.deserialize_next::<serde_json::Value>() {
                        Ok(raw_value) => match serde_json::from_value::<T>(raw_value) {
                            Ok(value) => return Some(Ok(value)),
                            // Silently skip malformed entries
                            Err(_) => continue,
                        },
                        Err(error) => return Some(Err(error.into())),
                    },
                    false => break,
                },
                Err(e) => return Some(Err(e.into())),
            }
        }
        if !self.closed {
            if let Err(error) = self.reader.end_array() {
                return Some(Err(error.into()));
            }
            self.closed = true;
        }
        None
    }
}

and this is how it is used:

    fn get_data_version(&self) -> Result<String, Box<dyn std::error::Error>> {
        let addr = Address::new("phobos", "metadata");
        let reader = self.get_reader(&addr)?;
        for metadata_result in ArrayIter::new(reader) {
            let metadata: PMetadata =
                metadata_result.map_err(|e| PhbFileEdhError::from_read_parse(e, addr.get_part_str()))?;
            if metadata.field_name == "client_build" {
                return Ok(metadata.field_value.to_string());
            }
        }
        Err(PhbFileEdhError::NoClientBuild.into())
    }

This implementation has some limitations which are useful for my use-case:

• it is not generic over type-stored-in-the-collection, or over collection type, which makes it much less complicated
• it does not parse directly into T, so does not need failure recovery code (but pays with performance for that)
• it skips serde parse errors (the only recoverable error here) silently; any error returned is an unrecoverable error, which makes error handling super easy on caller side

Generic iterator to cover more generic use-cases should be a bit more complex:

• iterators could be generic over type-stored-in-the-collection (u64 / serde-backed deserializable / string etc), or maybe even over collection type (but my guess is that it'd be tricky to implement, since processing JSON objects should yield some errors you will never see when working with JSON arrays)
• iterator's Item should still be Result<T, IterError>. IterError should provide a way for user to see if error is unrecoverable (iteration cannot proceed), or is recoverable (calling next() another time might yield an item)

Basic flow is:

• read opening delimiter: if any error, it is unrecoverable
• read a value (and its name if iterating over JSON object), and parse it using serde if necessary:
  • in case of various read errors, unrecoverable error is returned
  • if value is not what iterator expected it to be (e.g. it expected a number but read not a number; or it expected some data to be deserializable into T, but got something else), error recovery code is ran, and error of recoverable kind is returned, to notify that an item is of unexpected type. For arrays, it can mention index; for objects, errors should be different for field names and field values - if field name was not string, it returns one error; if value is of unexpected kind, it exposes field name under which unexpected object resides
• read closing delimiter: if any error, it is unrecoverable

A few open questions to settle with this approach:

• if reading of closing delimiter failed, should it really be unrecoverable error? If in some cases it might signify that data stream was corrupted (has_next() returned false, but not due to seeing a closing delimiter, but something else weird), I think it should.
• if an unrecoverable error occurs, what consequent calls to next() should return? None or Some(Err(unrecoverable))?

I don't have any strong opinions on those.

But overall, just having an iterator, and classification of errors gives me enough flexibility to decide if I want to break iteration, or skip malformed elements in calling code, and it is way better than current API for uniform collections imo.

[Marcono1234]

Thanks for the suggestion!

iterators could be generic over type-stored-in-the-collection (u64 / serde-backed deserializable / string etc)

Yes, hardcoding how the value is read would probably limit this to one specific use case and would not be useful enough for other users.

I am currently thinking about letting the user provide a value_reader: FnMut which takes a JsonReader and reads the value (e.g. simple value reading, deserialization, ...). Something like this:

fn next_array_iter<T, E: From<ReaderError>>(
    &mut self,
    // TODO: instead of `Option` use custom enum `Yield(Result<T, E>) | Continue`?
    value_reader: impl FnMut(&mut Self) -> Option<Result<T, E>>,
) -> Result<impl Iterator<Item = Result<T, E>>, ReaderError>;

The wrapping Option here is so that recovery or also simple value skipping can be performed within the value reader, and it can continue iteration by being called again if there are more elements. Passing the recovery back as iteration item might not be possible due to lifetime issues.

Usage would then look like this:

• simple

  let iter = json_reader.next_array_iter(|j| Some(j.deserialize_next::<u64>()))?;
  
  for result in iter {
      println!("{}", result?);
  }

• recoverable (with implementation from e190199)

  let iter = json_reader.next_array_iter(|j| {
      match j.deserialize_next_recoverable::<u8>() {
          Ok(v) => Some(Ok(v)),
          // unrecoverable
          Err((e, None)) => Some(Err(e)),
          Err((e, Some(recovery))) => {
              println!("encountered error; trying recovery: {e}");
              match recovery.recover_reader() {
                  Ok(()) => None, // iter should continue with next value
                  Err(e) => Some(Err(e.into())),
              }
          }
      }
  })?;

Full proof-of-concept implementation (click to expand)

// TODO doc: must exhaust iterator to fully consume array
// TODO doc: must abort on error, as mentioned in JsonReader doc, should therefore avoid methods like
//   `Iterator::skip` which would discard error
fn next_array_iter<T, E: From<ReaderError>>(
    &mut self,
    // TODO: instead of `Option` use custom enum `Yield(Result<T, E>) | Continue`?
    value_reader: impl FnMut(&mut Self) -> Option<Result<T, E>>,
) -> Result<impl Iterator<Item = Result<T, E>>, ReaderError> {
    // Already begin the array here to fail fast if next value is not array
    self.begin_array()?;

    #[derive(Debug)]
    enum IterState {
        Open,
        Closed,
        Failed,
    }
    #[derive(Debug)]
    struct ArrayIter<'j, J: ?Sized, F> {
        json_reader: &'j mut J,
        value_reader: F,
        state: IterState,
    }
    macro_rules! handling_err {
        ($self:expr, $reader_expr:expr) => {{
            let result = $reader_expr;
            match result {
                Err(e) => {
                    $self.state = IterState::Failed;
                    return Some(Err(e.into()));
                }
                Ok(v) => v,
            }
        }};
    }
    impl<
        J: JsonReader + ?Sized,
        T,
        // Require `From<ReaderError>` to support wrapping errors from `has_next()` and `end_array()` calls
        E: From<ReaderError>,
        F: FnMut(&mut J) -> Option<Result<T, E>>,
    > Iterator for ArrayIter<'_, J, F>
    {
        type Item = Result<T, E>;

        fn next(&mut self) -> Option<Self::Item> {
            if matches!(self.state, IterState::Closed | IterState::Failed) {
                return None;
            }

            while handling_err!(self, self.json_reader.has_next()) {
                let result = (self.value_reader)(self.json_reader);
                if let Some(result) = result {
                    if result.is_err() {
                        self.state = IterState::Failed;
                    }
                    return Some(result);
                }
                // continue with next
            }
            handling_err!(self, self.json_reader.end_array());
            self.state = IterState::Closed;
            None
        }
    }

    Ok(ArrayIter {
        json_reader: self,
        value_reader,
        state: IterState::Open,
    })
}

Would that be useful for you?

[Marcono1234]

maybe even over collection type (but my guess is that it'd be tricky to implement, since processing JSON objects should yield some errors you will never see when working with JSON arrays)

Yes, if that was implemented, it would probably be two dedicated methods (next_array_iter and next_object_iter). For JSON objects not sure if it would itself read the name as String already and pass that to the value_reader, or if it would take a member_reader: FnMut which is expected to consume the name itself (skipping or reading as str / String). The latter approach would be more flexible.

For now I would probably defer adding next_object_iter until there is demand for it.

---

if reading of closing delimiter failed, should it really be unrecoverable error?

Yes I think so. The JSON reader cannot be recovered, which is especially a problem when you continue using the reader afterwards to consume the data enclosing the iterated array. Treating the error as recoverable (or ignoring it) seems wrong.

---

if an unrecoverable error occurs, what consequent calls to next() should return? None or Some(Err(unrecoverable))?

Not sure what the conventions in the Rust ecosystem for this situation (that is, iterator returning Result) are. Returning Some(Err) once and then continuing with None might be most reasonable? If user code uses the iterator incorrectly (for the Struson use case here), e.g. calls Iterator::last or similar, continuing to return Some(Err) would cause an infinite loop.

[Marcono1234]

I am not completely sure whether adding this functionality is a good idea. My biggest concern is that this makes it easy to accidentally discard errors or not abort on them, in which case the Struson behavior is unspecified.
Especially methods like Iterator::skip or Iterator::last seem problematic. Dropping the iterator before exhausting it would also be error-prone, but then the behavior is at least not unspecified; most likely the user would get (confusing) errors when they are unexpectedly still inside the array. (Skipping the remaining elements on Drop is something I would like to avoid, due to being unable to properly handle errors.)

Could you please describe your use case a bit more in detail, especially why an iterator provides an advantage over a regular begin_array(), while has_next(), end_array().
For the example snippet with your get_data_version function above, I think it would not be much more verbose to use regular array reading.
I guess one advantage with your iterator implementation is that it hides away the automatic recovery. But as you mention, the Struson implementation should probably be generic, and therefore not perform automatic recovery itself. For the regular array reading, as user you could at least extract that into a separate helper function, to keep the main array reading loop concise.