Add support for resolving external entities

[adamreichold]

This adds minimal support for resolving external entities by enabling the user to supply a resolver which can act on the public ID and URI defining the entity.

Fixes #146

[adamreichold]

@PeterlitsZo Could you verify that this works for you? Thanks!

[PeterlitsZo]

OK, of course.

[PeterlitsZo]

I am glad to say that this works well for my HDFS configure file. Thanks a lot.

@RazrFalcon Hello, do you think if this PR can merged?

[adamreichold]

Hello, do you think if this PR can merged?

Note that he said in #123 that he might not be able to work on this crate at all. I do want to give him a week or so to chime in and veto this, but without input, we'll have to find a way forward ourselves.

[PeterlitsZo]

OK. Sounds great.

[RazrFalcon]

Looks good to me.

fmt::Debug for ParsingOptions looks ugly with that nested if. Better to extract it.

[adamreichold]

fmt::Debug for ParsingOptions looks ugly with that nested if. Better to extract it.

Done.

[Kimahriman]

Do you have an example of how this could be used with loading additional data from disk? The unit test uses a static str, and it's not clear how to create owned data in entity resolution and return a reference to it

[adamreichold]

Do you have an example of how this could be used with loading additional data from disk? The unit test uses a static str, and it's not clear how to create owned data in entity resolution and return a reference to it

One option would be to use an arena, e.g.

let bump = bumpalo::Bump::new();

let mut entity_resolver = |pub_id: Option<&str>, uri: &str| {
    let path = uri_to_path(pub_id, uri);

    let buf = std::fs::read_to_string(&path)
        .map_err(|err| format!("Failed to read from {}: {}", path.display(), err))?;

    Ok(Some(bump.alloc_str(&buf)))
};

However, not that uri_to_path is very difficult to implement securely. A better approach would be to load all allowed external entities before starting to parse and putting them in a HashMap or something from which entity_resolver can just borrow.

[Kimahriman]

Trying to do a flavor of that still leads to lifetime issues

let mut entity_resolver = move |pub_id: Option<&str>, uri: &str| {
    let buf = uri.to_string();
    let alloc: &str = bump.alloc_str(&buf);
    Ok(Some(alloc))
};

error: lifetime may not live long enough
   --> rust/src/common/config.rs:214:13
    |
211 |         let mut entity_resolver = move |pub_id: Option<&str>, uri: &str| {
    |                                   --------------------------------------
    |                                   |                                    |
    |                                   |                                    return type of closure is std::result::Result<Option<&'2 str>, String>
    |                                   lifetime `'1` represents this closure's body
...
214 |             Ok(Some(alloc))
    |             ^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
    |
    = note: closure implements `Fn`, so references to captured variables can't escape the closure

[adamreichold]

Sorry, the move did not belong there.

[Kimahriman]

Without the move, bump doesn't live long enough

[adamreichold]

You definitely need to borrow from bump and need to store it as far outside as necessary so that it lives at least as long as your input. This definitely works, c.f. the discussions over at adamreichold/robinson#4.

[Kimahriman]

That conversation was because of Kimahriman/hdfs-native#260 which is what I'm trying to figure out how to improve. I think the only reason it works currently is because it uses raw pointers to create &'static str, which seems like a really hacky workaround to roxmltree not wanting to own anything. Still trying to figure out how alloc_str can work properly with the right lifetimes