Download and unzip an XML document

Question

I am working on an integration software. I will need to download a list of products and import it into my website. The list of products will be downloaded from Constants.EndPoint which contains a zipped xml document.

I will be downloading this file periodically (once every 4 hours)

I have written UnzipClient which downloads and extract the file and returns the XML document. The consumer is responsible for deserializing the XML and importing its content (I have not included the consumer code in here)

UnzipClient.cs

public class UnzipClient
{
    private static readonly HttpClient _httpClient;
    private static readonly Uri _endpointUri;

    static UnzipClient()
    {
        _httpClient = new HttpClient();
        _endpointUri = new Uri(Constants.EndPoint);
    }

    public async Task<(XmlDocument xmlDocument, string error)> GetXml()
    {
        try
        {
            var response = await _httpClient.SendAsync(new HttpRequestMessage(HttpMethod.Get, _endpointUri));
            error = IsValidResponse(response, "Get");
            if (!string.IsNullOrEmpty(error))
            {
                return (null, error);
            }

            var xml = await LoadXml(response);
            return (xml, "");
        }
        catch (Exception ex)
        {
            string error = $"Exception sending a Get request. Message: '{ex.Message}', InnerException: '{ex.InnerException}'";
            return (null, error);
        }
    }

    private string IsValidResponse(HttpResponseMessage response, string RequestType)
    {
        if (response == null)
        {
            return $"{RequestType} response is null";
        }
        else if (response.StatusCode != HttpStatusCode.OK)
        {
            return $"Invalid response to {RequestType} request. StatusCode: '{response.StatusCode}', Reason: '{response.ReasonPhrase}'.";
        }
        else if (response.Content == null)
        {
            return $"{RequestType} request, response.Content is null";
        }

        return string.Empty;
    }

    private async Task<XmlDocument> LoadXml(HttpResponseMessage response)
    {
        using (var zipStream = await response.Content.ReadAsStreamAsync())
        using (ZipArchive archive = new ZipArchive(zipStream))
        {
            if (archive.Entries != null && archive.Entries.Count >= 1)
            {
                using (var unzipStream = archive.Entries[0].Open())
                {
                    var xml = new XmlDocument();
                    xml.Load(unzipStream);
                    return xml;
                }
            }
        }

        return null;
    }
}

The code works, but I am a little confused with the using blocks. Stream and ZipArchive are both Disposable so I would like to dispose them as soon as possible... however the consumer still needs to works with the xml document, so not sure if disposing of the unzipStream would have any impact on xml document?

Answer 1

Current Code Notes :

1. UnzipClient is not descriptive enough. Since the actual class is projected to one purpose and provider, then it would be better if you name it after the provider name and the provider api section if any. The goal here is to let anyone know the class purpose without the need to dig inside the class code.
2. the static constructor is unneeded. along with the static Uri
3. The Task<(XmlDocument xmlDocument, string error)> it's fine, however, I would prefer a user-defined class. It will give you more maintainability, readability, and extendibility. Or you can just return Task<XmlDocument> and throw exceptions when needed.
4. XmlDocument if is it self-choice, I would suggest using XDocument instead (AKA LINQ to XML). More readable, and easier to work with.
5. RequestType you can use HttpMethod instead.
6. LoadXml and IsValidResponse should be moved inside the main method, because it does not do anything outside that scope.
7. When using HttpClient it is a good idea to make use of BaseAddress instead of passing the full path on each request. This would be useful if for some reason the host is changed, then you only need to update BaseAddress
8. HttpClient has GetAsync why not use it instead of the current SendAsync?.

Here is an example that demonstrates the above notes :

public class XmlUnzipClientResult
{
    public int StatusCode { get; }  
    public bool IsSuccess { get; }  
    public XmlDocument Result { get; }  
    public string Message { get; }  
    public Exception ExceptionError { get; }
    
    public XmlUnzipClientResult(int statusCode, bool isSuccess, XmlDocument result, string message, Exception exception)
    {
        StatusCode = statusCode;
        IsSuccess = isSuccess; 
        Result = result;
        Message = message;
        ExceptionError = exception;
    }
    
    public static XmlUnzipClientResult Success(XmlDocument result)
    {
        return new XmlUnzipClientResult(200, true, result, null, null);
    }
    
    public static XmlUnzipClientResult Failure(int statusCode, string message, Exception exception = null)
    {
        return new XmlUnzipClientResult(statusCode, false, null, message, exception);
    }   
}

public class XmlUnzipClient  : IDisposable
{
    private readonly HttpClient _httpClient = new HttpClient();
    private readonly Uri _endpointUri = new Uri(Constants.EndPoint);
    
    public async Task<XmlUnzipClientResult> GetRequestResult()
    {
        
        try
        {
            var response = await _httpClient.GetAsync(_endpointUri);
            
            if(response.StatusCode != HttpStatusCode.OK)
            {
                return XmlUnzipClientResult.Failure((int)response.StatusCode, $"Invalid response to {HttpMethod.Get} request. StatusCode: '{response.StatusCode}', Reason: '{response.ReasonPhrase}'.");            
            }
            
            using (var zipStream = await response.Content.ReadAsStreamAsync())
            using (var archive = new ZipArchive(zipStream))
            {
                var entry = archive.Entries?.FirstOrDefault();

                if(entry != null)
                {
                    using (var unzipStream = entry.Open())
                    {
                        var xml = new XmlDocument();
                        xml.Load(unzipStream);
                        return XmlUnzipClientResult.Success(xml);
                    }

                }
            }
        
        }
        catch (Exception ex)
        {
            return XmlUnzipClientResult.Failure(500, $"'{ex.Message}'", ex);
        }
        
        return XmlUnzipClientResult.Failure(500, $"Unexpected Error");
    }
    
    #region IDisposable

    private bool _disposed;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                _httpClient.Dispose();
            }

            _disposed = true;
        }
    }

    #endregion  
}

the usage would be something like :

using(var client = new XmlUnzipClient())
{
    var results = await client.GetRequestResult();
    
    if(results.IsSuccess)
    {
        // do something.
    }
}

If there is multiple calls, you can declare a private static XmlUnzipClient in the class that will do that calls.

These are just to give some insights on what you've already done. However, from the given context, I don't beleive you need a class for that, it will be better to use extension methods for the ZipArchive which would add more useability to your project for more wider scope.

So, you need an extension method on HttpResponseMessage to return ZipArchive and another one on ZipArchiveEntry to return XmlDocument

Example :

public static class ZipArchiveExtensions
{
    public static async Task<ZipArchive> ReadAsZipArchiveAsync(this HttpResponseMessage response)
    {
        if (response == null) throw new ArgumentNullException(nameof(response));

        try
        {
            using (var zipStream = await response.Content.ReadAsStreamAsync())
            using (ZipArchive archive = new ZipArchive(zipStream))
            {
                return archive;
            }
        }
        catch (Exception)
        {
          // handle exceptions
        }

        return null;
    }

    public static XmlDocument ToXmlDocument(this ZipArchiveEntry entry)
    {
        if(entry == null) throw new ArgumentNullException(entry);

        try
        {
            var xml = new XmlDocument();
            
            xml.Load(entry.Open());
            
            return xml;
        }
        catch(Exception)
        {
          // handle exceptions
        }       

        return null;
    }
}

with that you can do this :

XmlDocument xml; 
using (var archive = await response.ReadAsZipArchiveAsync())
{
    var entry = archive.Entries?.FirstOrDefault();

    if(entry != null)
    {
        xml = entry.ToXmlDocument();
    }               
}

if(xml != null)
{
    // success do something
}

Answer 2

Disposing of the unzipStream, archive and zipStream objects when exiting the using blocks, won't affect the xml document object.

And a few notes:

1. Constants.EndPoint - I guess it is defined somewhere in the sources. I suggest to retrieve information about the endpoint from a configuration file, so that you don't need to re-compile the sources, if the endpoint changes.

2. It's a good practice to only catch exceptions that you can handle (not Exceptions).

3. As is your UnzipClient is not thread-safe because of the static objects, so if thread-safety is a requirement, you need to revise using them.

4. (cosmetic) ...&& archive.Entries.Count >= 1) - use just > 0, it's shorter and more understandble.

5. (cosmetic) from the readability perspective, I would use var for archive as we can see the type of the object and the actual type for zipStream instead of var.

   using (var zipStream = await response.Content.ReadAsStreamAsync())

   using (ZipArchive archive = new ZipArchive(zipStream))