Improving IRI handling using `oxiri`

[filippodebortoli]

At the moment, horned comes with its own IRI data structures IRI and IRIString, plus related traits ForIRI and WithIRI, methods for IRI resolution, and so on.

I found out that the @oxigraph project provides the oxiri crate, which comes with most of the features that horned seems to need.

@phillord do you think horned could benefit from using oxiri for handling IRIs or would you stick with the current implementation?

[althonos]

This sounds good too since horned uses the curie crate ATM, which is not super actively maintained...

[phillord]

Interesting. I think it is worth discussing the needs behind this first, rather than the specifics of the implementation.

Currently, IRI does two things -- at a trait level it is a new type around, well, mostly Borrow<str> with a pile of other traits; and, in implementation, it allows interning via Rc and Arc.

This addition changes IRI to allow do IRI validation as well as giving easy access to all various components of the IRI. I guess this is the key point in it's favour. This makes the library a bit stricter, but improves the ergonomics of IRI manipulation. Oxiri, IIUC, caches all the validation data, so if we check for protocol or host or whatever multiple times, that computation is shared.

There are several against as well.

It makes the ergonomics of all the Build methods more complex, because it requires error handling.

It makes some lazy code useful for testing not work:

use horned_owl::model::*;
fn main() {
    let b = Build::new_rc();
    println!("{:?}", b.class("123"))
}

Similarly, it would make parsing OWL which made use of invalid IRIs impossible, since the validation is there at a type level.

We could achieve some of the advantages without the cost. So, if model::IRI implemented TryInto<Iri>, we could get all the validation and so forth, without the changed interface to Build. But, we'd loose the caching of IRI components and the IRI validation would be off by default.

Curie vs oxiri is a slightly separate issue. The current PR doesn't replace curie (although it probably could do). And we could replace curie with oxiri without changing model.

Have I covered everything? In general, it seems like not a bad idea. I am a bit concerned about the ergonomics, since this is a drip-drip kind of thing. On it's own it doesn't seem to bad, but it's easy to make a library harder to use bit at a time. I am not sure about whether IRI validation by design would ever be a problem; I don't know if the OWL API can output invalid IRIs, but there are bound to be some tools out there that do.

@jannahastings Worthing asking Janna also -- this will pass through to python.

[phillord]

A lot of this could be achieved by having ForIRI implement TryFrom<String> rather than From<String>. That would make this a compile time choice or nearly so -- AnonymousIndividual breaks it slightly but that might be fixable, although it might hopeless complicate the already unwieldy generics.

[filippodebortoli]

Interesting. I think it is worth discussing the needs behind this first, rather than the specifics of the implementation.

Currently, IRI does two things -- at a trait level it is a new type around, well, mostly Borrow<str> with a pile of other traits; and, in implementation, it allows interning via Rc and Arc.

In case IRI validation became a must-have (or want-to-have), it would be possible to switch out model::IRI and replace it with oxiri::Iri altogether. The only bound over oxiri::Iri<A> is A: Deref<Target = str>, and trait implementations are provided by oxiri for most of the bounds or traits induced from model::ForIRI.

There are several against as well.

It makes the ergonomics of all the Build methods more complex, because it requires error handling.

Fair enough. The quickest workaround I can think of is to implement Build::iri as follows:

    pub fn iri<S: Borrow<str> + Deref<Target=str>>(&self, iri: S) -> IRI<A> {
        let mut cache = self.0.borrow_mut();
        if let Some(cached_iri) = cache.get(iri.borrow()) {
            Ok(cached_iri.clone())
        } else {
            let parsed_iri: Iri<A> = Iri::parse(iri.borrow().to_string().into())
                                     .expect("Invalid IRI found."); // <-- CHANGE HERE.
            cache.insert(parsed_iri.clone().into());
            Ok(parsed_iri.into())
        }
    }

This would remove the need for most of the error management and still inform the user whenever they are trying to parse an invalid IRI.

We could achieve some of the advantages without the cost. So, if model::IRI implemented TryInto<Iri>, we could get all the validation and so forth, without the changed interface to Build. But, we'd loose the caching of IRI components and the IRI validation would be off by default.

What do you mean here? Would you want to add a IRI::validate(&self) -> Result<Iri<A>, IriParseError> and leave everything else as it was before?

Curie vs oxiri is a slightly separate issue. The current PR doesn't replace curie (although it probably could do). And we could replace curie with oxiri without changing model.

How is curie currently used within horned? Aren't CURIEs and IRIs different things anyway?

EDIT: yet another alternative could be to use the sophia_iri crate, which allows the handling of unchecked IRIs through Iri::new_unchecked() and allows for validation and resolution using oxiri as the backend().

[phillord]

Oh dear, did I get confused half way through.

If you move model::IRI to have TryFrom<String> instead of From<String> then we'd be nearly able to do this with a generic, but it is only the fallability of construction that distinguishes model::IRI from oxiri::Iri.

Another solution would be to make a IriMaybe struct that would like a bit like this (untested and probably uncompilable) sketch:

struct IriMaybe<A:ForIRI> {
      raw: A,
      cached_iri: Option<Iri>
} 

impl TryInto<&Iri> for IriMaybe {
      fn try_into(&self) -> Result<&Iri, Error> {
             match &self.cached_iri {
                   Some(iri) => &iiri,
                   None => {
                         let iri = Iri::parse(&self.raw)?;
                         self.cached_iri = Some(iri);
                         self.try_into()
                   }
       }
}

We now have a struct which we can infallably convert from a String but which will fallably give us access to all the methods in Iri. So we pay the cost only if we use it. If we don't the string is never parsed. So, it will fit directly into the generic space that we have for IRI at the moment. Also for AnonymousIndividual, although it will fail to parse in most of these cases.

On curie vs oxiri -- I only use this in one of the readers. Perhaps they are different, I don't know. Worth checking.

EDIT: hmmm, is the sophia_iri crate doing what I suggested above?

[jannahastings]

I don't have any thoughts on the best strategy for the Iri library. I trust @phillord will know best :-) and will be ready to update py-horned-owl as needed. However, I just wanted to add that I use the curie perhaps more extensively in the Python library as I provide access and lookup via CURIE-form IDs.

[filippodebortoli]

If you move model::IRI to have TryFrom<String> instead of From<String> then we'd be nearly able to do this with a generic, but it is only the fallability of construction that distinguishes model::IRI from oxiri::Iri.

I am confused by this sentence.
I understood that you are proposing to replace From<String> with TryFrom<String> in the ForIRI trait.
In the context of the PR, where

pub struct IRI<A>(Iri<A>);

it is unclear to me how having TryFrom<String> would make things easier or more ergonomic.
In the context of the current implementation, where

pub struct IRI<A>(A);

I think that implementing TryInto<IRI<A>> would make sense:

impl<A: ForIRI> TryInto<Iri<A>> for IRI<A> {
    type Error = oxiri::IriParseError;
    fn try_into(self) -> Result<Iri<A>, Self::Error> {
        Iri::parse(self)
    }
}

Notice that the TryInto trait exposes try_from(self) and not try_from(&self), which means that converting IRI<A> to Iri<A> this way would consume the input. Would this be OK? By looking at the crate code, I have the impression it would be not.

Another solution would be to make a IriMaybe struct that would like a bit like this (untested and probably uncompilable) sketch:

struct IriMaybe<A:ForIRI> {
      raw: A,
      cached_iri: Option<Iri>
} 

impl TryInto<&Iri> for IriMaybe {
      fn try_into(&self) -> Result<&Iri, Error> {
             match &self.cached_iri {
                   Some(iri) => &iiri,
                   None => {
                         let iri = Iri::parse(&self.raw)?;
                         self.cached_iri = Some(iri);
                         self.try_into()
                   }
       }
}

This solution would work if try_into(&mut self) was available, but it is not.

EDIT: hmmm, is the sophia_iri crate doing what I suggested above?

Kinda. They expose pub struct Iri<'a>(&'a str) though, so interning might be lost by using their implementation.

[phillord]

I will knock up a rough implementation of IriMaybe and see if that will make it clearer. I don't have any data at the moment, but having the IRI validation on by default seems like it will carry a significant load. I guess that also is worth testing. How long would it take to validate the 100,000 or so IRIs that appear in the GO file?

[phillord]

Like this.

https://github.com/phillord/horned-owl/tree/scratch/oxiri-adapted

Little or no runtime cost, access to all the oxiri methods, in a (mostly) cached way, but does not validate at point of parsing.

It should be possible to have an eager oxiri wrapper which panics on validation. As noted, there is no way to make this error nicely, only panic. We would have to change ForIRI to TryFrom<String>; this has the same ergonomics as your approach, but means the validation is not hard coded.

Still, with an eager wrapper, we could performance test this against some real world examples.

[phillord]

@jannahastings yeah, I'd forgotten the curie look up stuff. I'll have a look through the code to see what can be done. I'd rather not having two IRI libraries floating around, but it might be a necessity at the moment.

[filippodebortoli]

Like this.

https://github.com/phillord/horned-owl/tree/scratch/oxiri-adapted

Little or no runtime cost, access to all the oxiri methods, in a (mostly) cached way, but does not validate at point of parsing.

It should be possible to have an eager oxiri wrapper which panics on validation. As noted, there is no way to make this error nicely, only panic. We would have to change ForIRI to TryFrom<String>; this has the same ergonomics as your approach, but means the validation is not hard coded.

Still, with an eager wrapper, we could performance test this against some real world examples.

I am wondering if implementing IriMaybe as a struct rather than an enum is really necessary - I do not understand if caching is in any way lost when using enums. I spun an attempt at having IriMaybe as an enum here (all the branch here).

[phillord]

If we do this, though, we have to choose at construction time whether we have validated or not. If our ontology starts with unvalidated IRIs, it will always remain so. The best we can do then is iri.clone().to_validated() right? That doesn't seem much easier to me than oxiri::Iri::parse(iri.underlying()) which does the same thing. If this is what we want, adding a to_oxiri method to iri should do the trick.

With my approach, the IRIMaybe can be queried at any point and once it is, the change happens in place. So we could fetch the oxiri at may places in code. But, it is adding a lot of complexity for something we are not likely to do that often, and if we really want to cache the model::IRI -> oxiri::Iri mapping it is just a HashMap away.

Is the real solution one a to_oxiri method for IRI? Very simple, very small.