Improve XLTemplate API (#16)

* Improve XLTemplate API

* Do not dispose wokbook if it was passed into XLTemplate constructor externally

Author: Pankraty

ClosedXML.Report/XLTemplate.cs

@@ -1,15 +1,21 @@
-using System.Linq;
-using System.Reflection;
-using ClosedXML.Excel;
+using ClosedXML.Excel;
 using ClosedXML.Report.Excel;
 using ClosedXML.Report.Options;
+using System;
+using System.IO;
+using System.Linq;
+using System.Reflection;
 
 namespace ClosedXML.Report
 {
-    public class XLTemplate
+    public class XLTemplate : IDisposable
     {
-        private readonly XLWorkbook _workbook;
         private readonly RangeInterpreter _interpreter;
+        private bool _disposeWorkbookWithTemplate;
+
+        public bool IsDisposed { get; private set; }
+
+        public IXLWorkbook Workbook { get; private set; }
 
         static XLTemplate()
         {
@@ -46,15 +52,26 @@ static XLTemplate()
             TagsRegister.Add<SummaryFuncTag>("VARP");
         }
 
-        public XLTemplate(XLWorkbook workbook)
+        public XLTemplate(string fileName) : this(new XLWorkbook(fileName))
+        {
+            _disposeWorkbookWithTemplate = true;
+        }
+
+        public XLTemplate(Stream stream) : this(new XLWorkbook(stream))
+        {
+            _disposeWorkbookWithTemplate = true;
+        }
+
+        public XLTemplate(IXLWorkbook workbook)
         {
-            _workbook = workbook;
+            Workbook = workbook ?? throw new ArgumentNullException(nameof(workbook), "Workbook cannot be null");
             _interpreter = new RangeInterpreter(null);
         }
 
         public void Generate()
         {
-            foreach (var ws in _workbook.Worksheets.Where(sh => sh.Visibility == XLWorksheetVisibility.Visible && !sh.PivotTables.Any()).ToArray())
+            CheckIsDisposed();
+            foreach (var ws in Workbook.Worksheets.Where(sh => sh.Visibility == XLWorksheetVisibility.Visible && !sh.PivotTables.Any()).ToArray())
             {
                 ws.ReplaceCFFormulaeToR1C1();
                 _interpreter.Evaluate(ws.AsRange());
@@ -64,6 +81,7 @@ public void Generate()
 
         public void AddVariable(object value)
         {
+            CheckIsDisposed();
             var type = value.GetType();
             var fields = type.GetFields(BindingFlags.Public | BindingFlags.Instance).Where(f => f.IsPublic)
                 .Select(f => new { f.Name, val = f.GetValue(value), type = f.FieldType })
@@ -78,7 +96,37 @@ public void AddVariable(object value)
 
         public void AddVariable(string alias, object value)
         {
+            CheckIsDisposed();
             _interpreter.AddVariable(alias, value);
         }
+
+        public void SaveAs(string file)
+        {
+            CheckIsDisposed();
+            Workbook.SaveAs(file);
+        }
+
+        public void SaveAs(Stream stream)
+        {
+            CheckIsDisposed();
+            Workbook.SaveAs(stream);
+        }
+
+        public void Dispose()
+        {
+            if (IsDisposed)
+                return;
+
+            if (_disposeWorkbookWithTemplate)
+                Workbook.Dispose();
+            Workbook = null;
+            IsDisposed = true;
+        }
+
+        private void CheckIsDisposed()
+        {
+            if (IsDisposed)
+                throw new ObjectDisposedException("Template has been disposed");
+        }
     }
 }

tests/ClosedXML.Report.Tests/XlTemplateTests.cs

@@ -1,8 +1,10 @@
 using ClosedXML.Excel;
 using ClosedXML.Report.Tests.TestModels;
 using FluentAssertions;
+using NSubstitute;
 using System;
 using System.Collections.Generic;
+using System.IO;
 using System.Linq;
 using Xunit;
 using Xunit.Abstractions;
@@ -201,5 +203,83 @@ IEnumerable<dynamic> GenerateItems()
                 };
             }
         }
+
+        [Fact]
+        public void XLTemplateWithNoWorkbookFails()
+        {
+            IXLWorkbook wb = null;
+            Assert.Throws<ArgumentNullException>(() => new XLTemplate(wb));
+        }
+
+        [Fact]
+        public void XLTemplateOpenFromFile()
+        {
+            var fileName = Path.Combine(TestConstants.TemplatesFolder, "1.xlsx");
+            using (var template = new XLTemplate(fileName))
+            {
+                template.Workbook.Should().NotBeNull();
+                template.Workbook.Worksheets.First().FirstCell().Value.Should().Be("{{TestValue1}}");
+            }
+        }
+
+        [Fact]
+        public void XLTemplateOpenFromStream()
+        {
+            var fileName = Path.Combine(TestConstants.TemplatesFolder, "1.xlsx");
+            using (var stream = File.Open(fileName, FileMode.Open))
+            {
+                var template = new XLTemplate(stream);
+
+                template.Workbook.Should().NotBeNull();
+                template.Workbook.Worksheets.First().FirstCell().Value.Should().Be("{{TestValue1}}");
+            }
+        }
+
+        [Fact]
+        public void AutoCreatedWorkbookDisposedWithTemplate()
+        {
+            var disposed = false;
+            var wb = Substitute.For<IXLWorkbook>();
+            wb.When(w => w.Dispose()).Do(w => disposed = true);
+
+            var template = new XLTemplate(wb);
+
+            template.Dispose();
+            disposed.Should().BeFalse("Workbook specified in the constructor should not be disposed");
+        }
+
+        [Fact]
+        public void WorkbookNotDisposedWithTemplate()
+        {
+            var disposed = false;
+            var wb = Substitute.For<IXLWorkbook>();
+            wb.When(w => w.Dispose()).Do(w => disposed = true);
+
+            var fileName = Path.Combine(TestConstants.TemplatesFolder, "1.xlsx");
+            var template = new XLTemplate(fileName);
+            ReplaceWorkbookWithMock(template, wb);
+
+            template.Dispose();
+
+            disposed.Should().BeTrue("Workbook expected to be disposed");
+        }
+
+        [Fact]
+        public void AccessToDisposedTemplateThrows()
+        {
+            var fileName = Path.Combine(TestConstants.TemplatesFolder, "1.xlsx");
+            var template = new XLTemplate(fileName);
+
+            template.Dispose();
+
+            Assert.Throws<ObjectDisposedException>(() => template.AddVariable("Test", "test"));
+            Assert.Throws<ObjectDisposedException>(() => template.Generate());
+        }
+
+        private void ReplaceWorkbookWithMock(XLTemplate template, IXLWorkbook mock)
+        {
+            var property = template.GetType().GetProperty("Workbook");
+            property.SetValue(template, mock);
+        }
     }
 }

tests/ClosedXML.Report.Tests/XlsxTemplateTestsBase.cs

@@ -41,10 +41,8 @@ protected void XlTemplateTest(string tmplFileName, Action<XLTemplate> arrangeCal
 
             var fileName = Path.Combine(TestConstants.TemplatesFolder, tmplFileName);
             using (var stream = File.Open(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
+            using (var template = new XLTemplate(stream))
             {
-                var workbook = new XLWorkbook(stream);
-                var template = new XLTemplate(workbook);
-
                 // ARRANGE
                 arrangeCallback(template);
 
@@ -56,7 +54,7 @@ protected void XlTemplateTest(string tmplFileName, Action<XLTemplate> arrangeCal
                     template.Generate();
                     Output.WriteLine(DateTime.Now.Subtract(start).ToString());
                     //MemoryProfiler.Dump();
-                    workbook.SaveAs(file);
+                    template.SaveAs(file);
                     //MemoryProfiler.Dump();
                     file.Position = 0;
 
@@ -66,13 +64,10 @@ protected void XlTemplateTest(string tmplFileName, Action<XLTemplate> arrangeCal
                         assertCallback(wb);
                     }
                 }
-
-                workbook.Dispose();
-                workbook = null;
-                template = null;
-                GC.Collect();
-                //MemoryProfiler.Dump();
             }
+
+            GC.Collect();
+            //MemoryProfiler.Dump();
         }
 
         protected void CompareWithGauge(XLWorkbook actual, string fileExpected)