Orange-OpenSource/fqdn

Derive Ord for FQDN

Closed this issue · 13 comments

Currently it cannot be used in e.g. BTreeMap due to the lack of Ord. Inner Cstring has Ord so it should be simple.

It is not exactly the same ordering as Cstring since it should be case insensitive.
But it remains not so difficult.
Ok, I take the point.

Ah, yes, forgot about the case. Thanks, it would be good to have it!

Done in 0.3.6

Thanks a lot!

However, if you manipulate FQDN in maps, you should take a look to the crate fqdn-trie which provides a FqdnTrieMap with better performance than the standard map (BTreeMap or even HashMap).
In addition, this map could also find the longest domain that contains a given key.
But depends on why you need the order, of course.

Oh, cool, thanks, will look at it. My case is simple and probably BTreeMap is sufficient, but it's nice to take note of it.

Hey @XopheD that's me again.

It seems that there's some problem in the Ord/PartialOrd implementation.
It works nicely in my tests, but when it comes to production it fails to look up an entry in a BtreeMap, this is a bit strange and hard to debug/reproduce.

I have two instances of FQDN - one I create manually with fqdn! macro and another I'm parsing from HTTP Host header using FQDN::from_str().
One is looked up in BTreeMap successfully, the other does not, though they both seem to have the same byte structure:

fn lookup(&self, hostname1: &Fqdn) {
        let hostname2 = &fqdn!("rb-test0.icp1.io");
        println!("h1 {:?}", hostname1.as_bytes());
        println!("h2 {:?}", hostname2.as_bytes());
        println!("EQ {:?}", hostname1 == hostname2);

        println!("{:?}", self.btreemap.get(hostname1));
        println!("{:?}", self.btreemap.get(hostname2));
}

h1 [8, 114, 98, 45, 116, 101, 115, 116, 48, 4, 105, 99, 112, 49, 2, 105, 111, 0]
h2 [8, 114, 98, 45, 116, 101, 115, 116, 48, 4, 105, 99, 112, 49, 2, 105, 111, 0]
EQ true

None
Some(...})

So for hand-crafted the lookup succeeds, and the one from FQDN::from_str() it fails, though the internal byte representation looks identical.

If I replace BTreeMap with HashMap it works fine.

I wonder what's wrong there.

Could you send to me a standalone piece of code that produces this issue ?
I tried to write it but without success.

That's the problem - I also couldn't do it outside my service. Both fqdn! and FQDN::from_str from a &str produce the same working results, but the FQDN that I extract from an HTTP request fails. I'll try to find a way to do it.

More or less the FQDN is produced like this:

fn extract_authority(request: &http::Request) -> Option<FQDN> {
    request
        .uri()
        .authority()
        .map(|x| x.host())
        // Split if it has a port
        .and_then(|x| x.split(':').next())
        .and_then(|x| FQDN::from_str(x).ok())
}

I've tried examining x that I'm using to build FQDN and it's byte-identical to what it should be and to the contents of the resulting FQDN (well, accounted for dots replaced with the length prefixes and null termination).

If I'm producing the http::Request in tests then it also works flawlessly. Looks like voodoo to me :)

Btw fqdn-trie also works fine, though I guess it does not depend on the Ord implementation since it wasn't there until recently.

Ok, though I don't yet have a working repro, it seems to have something to do with Deref and FQDN <-> Fqdn relations.

fn lookup2(&self, hostname: &Fqdn) -> Option<..> {
        self.btreemap.get(hostname).copied()
}

fn lookup(&self, hostname1: &Fqdn) {
        let hostname2 = &fqdn!("foo.bar");
        println!("{:?}", self.btreemap.get(hostname1));
        println!("{:?}", self.btreemap.get(hostname2));
        println!("{:?}", self.lookup2(hostname2));
}
None
Some(...})
None

So the direct lookup succeeds, but when passed into a method it fails. Though in tests I still can't make this happen.

Ok it seems that I've got a working repro, it seems to be dependent on map contents and needs passing a reference:

use fqdn::{fqdn, Fqdn, FQDN};
use std::{collections::BTreeMap, str::FromStr};

fn get(h: &Fqdn, t: &BTreeMap<FQDN, i32>) -> Option<i32> {
    t.get(h).copied()
}

fn main() {
    let mut t = BTreeMap::new();
    t.insert(fqdn!("3foo.xyz"), 1);
    t.insert(fqdn!("rbar.baz"), 2);

    let fqdn1 = &fqdn::fqdn!("rbar.baz");
    let fqdn2 = fqdn::FQDN::from_str("rbar.baz").unwrap();

    println!("{:?}", t.get(fqdn1));
    println!("{:?}", get(fqdn1, &t));
    println!("{:?}", get(&fqdn2, &t));
    println!("{:?}", get(&fqdn!("rbar.baz"), &t));
}
Some(2)
None
None
None

I've got it ! (see v0.3.10)
There was an inconsistency between Ord and PartialOrd.
Thank you for your repro.

Thanks a lot!