vec impl for DnsResolver

[parkma99]

Hi @GlenDC I have a try for #332 But I am not sure my implement is totally that issue asked. And also, thanks for your helping

[GlenDC]

Pretty much spot on. Hope my comments serve as the missing guidelines you were needing. However, you do seem to have understood the intent of the issue as far as I can see. So great start!

[parkma99]

Thanks @GlenDC , I make some change in recent commits. I would like to add some tests, can you give me some help?

[GlenDC]

Thanks @GlenDC , I make some change in recent commits. I would like to add some tests, can you give me some help?

Sure. What tests would you like to add? I can chip in if they are useful and how you might go on doing so.

[GlenDC]

Current version looks good except the minor comments I made.
With these changes applied I'm good with merging it, but curious to hear what kind of tests you want to still add.

[parkma99]

Sure. What tests would you like to add? I can chip in if they are useful and how you might go on doing so.

Some demos for using DnsChainDomainResolve, just like in #332 mentioned

[GlenDC]

Sure. What tests would you like to add? I can chip in if they are useful and how you might go on doing so.

Some demos for using DnsChainDomainResolve, just like in #332 mentioned

Sure. For general advise you can check existing tests in rama. E.g. how to do multiple cases of same tests (test cases), or fact that you'll need to use tokio::test for async tests, etc...

Regarding these tests. Wouldn't overdo it. Again use your own judgement, but following cases come to my mind:

1. test empty array/vec, will result in error but without inner errors (and that's ok)
2. test [Ok, Err]
3. test [Err, Ok]
4. test [Ok, Ok]
5. test [Err, Err, Ok]
6. test [Err, Err]

2-6 for both vec/array I guess.

You can use the existing https://ramaproxy.org/docs/rama/dns/struct.DenyAllDns.html for your err case, and https://ramaproxy.org/docs/rama/dns/struct.InMemoryDns.html for the ok case. I wouldn't check on the actual inner errors, just the high level Ok vs Err is good enough :) And if ok of course do check if you get the expected IP. Make sure to test both IPv4 and IPv6.

Feel free to cut on testing where you see fit, logic is simple enough that I don't think we really need that much testing here, but given you seem motivated to do so, do feel free to go for it :)

[parkma99]

Hi @GlenDC ,I still have no idea for testing, could you give me some example code , thanks

[GlenDC]

If you do E: std::error::Error + 'static, can you not just do
self.errors.last() on line 101? If not what error do you get?

[GlenDC]

I think it might be a good idea to move all your new code here to a new file: rama-dns/src/chain.rs. Can be a private fail of which you pub use (re-export) types here.

[GlenDC]

You can refactor these 2 definitions as:

macro_rules! dns_resolver_chain_impl {
    () => {
        async fn ipv4_lookup(&self, domain: Domain) -> Result<Vec<Ipv4Addr>, Self::Error> {
            let mut errors = Vec::new();
            for resolver in self {
                match resolver.ipv4_lookup(domain.clone()).await {
                    Ok(ipv4s) => return Ok(ipv4s),
                    Err(err) => errors.push(err),
                }
            }
            Err(DnsChainDomainResolveErr { errors })
        }
    
        async fn ipv6_lookup(&self, domain: Domain) -> Result<Vec<Ipv6Addr>, Self::Error> {
            let mut errors = Vec::new();
            for resolver in self {
                match resolver.ipv6_lookup(domain.clone()).await {
                    Ok(ipv6s) => return Ok(ipv6s),
                    Err(err) => errors.push(err),
                }
            }
            Err(DnsChainDomainResolveErr { errors })
        }
    }
}

so that you can do:

impl<R, E> DnsResolver for Vec<R>
where
    R: DnsResolver<Error = E> + Send,
    E: Send + 'static,
{
    type Error = DnsChainDomainResolveErr<E>;
    
    dns_resolver_chain_impl!()
}

impl<R, E, const N: usize> DnsResolver for [R; N]
where
    R: DnsResolver<Error = E> + Send,
    E: Send + 'static,
{
    type Error = DnsChainDomainResolveErr<E>;
    
    dns_resolver_chain_impl!()
}

Might give you an error around scoping or w/e, but nothing that can be resolved by in worst case
passing some stuff. With some cleverness you can probably even make it shorter, but this is already sufficient.

[GlenDC]

while you're at it, feel free to move this macro def and its usage to a separate file called rama-dns/src/variant.rs, keeps the lib.rs pretty clean

[GlenDC]

Hi @GlenDC ,I still have no idea for testing, could you give me some example code , thanks

Sure, in rama-dns/src/in_memory.rs there are already some example tests.

So to pick up some of the given example cases, you can start with:

#[cfg(test)]
mod tests {
    use std::net::{Ipv4Addr, Ipv6Addr};
    use rama_core::combinators::Either;

    use super::*;

    #[tokio::test]
    async fn test_empty_chain_vec() {
        let v = Vec::<DnsOverwrite>::new();
        assert!(v
            .ipv4_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
        assert!(v
            .ipv6_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
    }

    #[tokio::test]
    async fn test_empty_chain_array() {
        let a: [DnsOverwrite; 0] = [];
        assert!(a
            .ipv4_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
        assert!(a
            .ipv6_lookup(Domain::from_static("plabayo.tech"))
            .await
            .is_err());
    }

    #[tokio::test]
    async fn test_chain_ok_err_ipv4() {
        let v = vec![
            Either::A(serde_html_form::from_str("example.com=127.0.0.1").unwrap()),
            Either::B(DenyAllDns::new()),
        ];
        assert!(
            v.ipv4_lookup(Domain::from_static("example.com"))
                .await
                .unwrap()
                .into_iter()
                .next()
                .unwrap(),
            Ipv4Addr::new(127, 0, 0, 1)
        );
        assert!(v
            .ipv6_lookup(Domain::from_static("example.com"))
            .await
            .is_err());
    }
    
    #[tokio::test]
    async fn test_chain_err_err_ok_ipv6() {
        // ... TODO
    }
    
    // TODO ...
}

Haven't tested the code or ran it so you might need to solve some issues in the code, but hopefully it gets the idea across. I certainly wouldn't test all possible combinations. Just try to combine a couple so that you more or less have tested some couple things. I'm not interested in full test coverage, so try to be minimal and smart about it, as this is also all code to maintain in the end. And thanks to how we write the code, e.g. using a macro for a single implementation, it is already more guaranteed that we have working code for both sequence implementations.

[GlenDC]

Hi @parkma99 hope you are doing fine. Do you want to hand over this issue to someone else? Or do you want to continue yourself. In case of the latter, are you in need of assistance or help? Or mentoring of any kind?

[parkma99]

Hi @GlenDC I want to hand over this issue to someone else because of busy working 😫😫

[GlenDC]

Hi @GlenDC I want to hand over this issue to someone else because of busy working 😫😫

All good. Not a problem at all. Take care remote friend. Hope your work and rest of your life goes well!