Fix breaking dll rename of SharpZipLib - issue #2095 #2118

iJungleboy · 2018-06-12T20:28:36Z

Summary

Fixes #2095 by registering an assembly-resolver that is used by .net when an assembly isn't found. So if something tries to get SharpZipLib.dll and fails, this resolver will kick in and tell .net that it's in the new assembly ICSharpCode.SharpZipLib.

I also added a readme.md explaining it some more, and the "register assembly handler" must happen very early, which is why I needed to put it into the application start.

sync from base

tpluscode

Thanks for this solution. Very clean 👍

I suppose we could revert #2019 once this is merged?

tpluscode · 2018-06-13T14:55:50Z

DNN Platform/Library/Services/Zip/SharpZipLibRedirect.cs

+            }
+            catch
+            {
+                /* ignore */


In what circumstances is an exception here possible? Swallowing it will mean that the redirect will not fire, right?

Yes, this is to ensure that if anything at all goes wrong when registering this, it won't break DNN.

tpluscode · 2018-06-13T14:57:05Z

DNN Platform/Library/Services/Zip/SharpZipLibRedirect.cs

+        {
+            // only check for access to old SharpZipLib
+            if (!args.Name.StartsWith(OldName))
+                return null;


Please add braces

is that the DNN convention? or your personal preference? just to be sure...

Not a DNN convention, given that no official conventions exist. It is however a sensible choice always to add curly brackets and also a StyleCop inspection, enabled by default I think

tpluscode · 2018-06-13T14:57:12Z

DNN Platform/Library/Services/Zip/SharpZipLibRedirect.cs

+        /// </summary>
+        public static void RegisterSharpZipLibRedirect()
+        {
+            if (AlreadyRun) return;


Please add braces

DNN convention?

bdukes · 2018-06-13T15:07:38Z

@tpluscode it doesn't look like #2019 was ever merged

tpluscode · 2018-06-13T15:12:58Z

Oh, true. Good :)

bdukes · 2018-06-13T15:11:02Z

DNN Platform/Library/Services/Zip/SharpZipLibRedirect.cs

+        private static Assembly Handler(object sender, ResolveEventArgs args)
+        {
+            // only check for access to old SharpZipLib
+            if (!args.Name.StartsWith(OldName))


please add StringComparison.Ordinal as second parameter to StartsWith

Is there a reason this is using StartsWith instead of Equals?

ok, will do. I think I originally used the name without DLL, so equals should be ok. i'll change it.

I found out that it needs starts with, because the real string looks something like this SharpZipLib, Version=0.81.0.1407, Culture=neutral, PublicKeyToken=null.

I get this error, if I use equals (because the mapping then fails):

bdukes · 2018-06-13T15:11:08Z

DNN Platform/Library/Services/Zip/SharpZipLibRedirect.cs

+    /// final solution taken from https://raw.githubusercontent.com/2sic/2sxc/master/2sxc%20Dnn/Dnn920/SharpZipLibRedirect.cs
+    /// read the readme.md in this folder for more info
+    /// </remarks>
+    public class SharpZipLibRedirect


I'm not real excited about this being public, seems like it will confuse folks that there's a DotNetNuke.Services.Zip namespace that doesn't have anything useful for them inside of it. Should this just live next to (or inside of) DotNetNukeHttpApplication?

I agree, internal would be better if we can

ok great suggestion! I found that I can make it internal

bdukes · 2018-06-13T15:11:51Z

DNN Platform/Library/Services/Zip/readme.md

@@ -0,0 +1,11 @@
+# Zip Services in DNN
+
+DNN uses SharpZipLib as the zip system of choice. Because of this, it is bundled with DNN. 


I would prefer that this be included as comments in the SharpZipLibRedirect class, rather than as a markdown document.

I prefer markdown - because often explanations will need multiple code files, but I'll put the same info into the class file as well - ok?

For consistency with the rest of the project I would ONLY have this in the code comments. Multi-line XML comments can support HTML formatting and will be automatically built into the documentation. THis is used universally across the entire solution

@mitchelsellers ok, I just moved all the docs into code-comments only

iJungleboy · 2018-06-14T06:38:20Z

Thanks for the thorough review! I'm updating everything as discussed, after testing I'll republish...

iJungleboy · 2018-06-14T06:50:37Z

Changes made and published, pls re-check, thx!

ohine · 2018-06-15T01:59:45Z

I'm guessing we need to update this function as well with both assembly names? or change the StartsWith to Contains? Right @bdukes?

Dnn.Platform/DNN Platform/Library/Framework/Reflections/TypeLocator.cs

Lines 105 to 112 in 6387dd1

    
           bool canScan = !(assemblyName.StartsWith("clientdependency.core") || assemblyName.StartsWith("countrylistbox") 
        
               || assemblyName.StartsWith("icsharpcode") || assemblyName.StartsWith("fiftyone") 
        
               || assemblyName.StartsWith("lucene") || assemblyName.StartsWith("microsoft") 
        
               || assemblyName.StartsWith("newtonsoft") || assemblyName.StartsWith("petapoco") 
        
               || assemblyName.StartsWith("sharpziplib") || assemblyName.StartsWith("system") 
        
               || assemblyName.StartsWith("telerik") || assemblyName.StartsWith("webformsmvp") 
        
               || assemblyName.StartsWith("webmatrix") || assemblyName.StartsWith("solpart") 
        
               );

iJungleboy · 2018-06-15T13:17:13Z

@tpluscode @bdukes @ohine I believe I did everything you guys asked for. Ok for now, or anything still pending?

tpluscode · 2018-06-15T14:09:35Z

DNN Platform/Library/Services/Zip/SharpZipLibRedirect.cs

+    /// </remarks>
+    internal class SharpZipLibRedirect
+    {
+        internal static bool AlreadyRun { get; private set; }


Hm, I notice this changed from public to internal. But does it really have to be accessible from anywhere? I'd expect such property to be private

I would leave it in for debugging - in case this is ever questioned. But I can change it if that is preferred.

No strong opinion here. Let's leave it as is

bdukes · 2018-06-15T15:25:20Z

Good catch @ohine, I'd just add both

iJungleboy · 2018-06-16T05:35:34Z

@bdukes and @ohine I don't understand that part about the other code. Should I do something there? I've never used/seen that code before.

ohine · 2018-06-17T12:30:01Z

@iJungleboy I've never seen that code either, I was researching the SQL dependency issue (#2116) and stumbled across that function in the process. We should add both of the assembly names to that function. If you can do it in this pull request that would be great, if not I'll start up another pull request with that change.

iJungleboy · 2018-06-19T11:54:14Z

@ohine I would rather not touch that - I don't know anything about it and it's not related to the sharpziplib issue directly - ok?

I believe we've covered everything - shall we finish the pull @bdukes and @tpluscode ?

mitchelsellers · 2018-06-19T13:13:51Z

I think that we need determine WHY those DLL's are excluded from the scanning process before we fully implement this. For a DLL to be explicitly listed here my guess is that there was an issue.

@ohine or @bdukes any idea on how we might track this down?

ohine · 2018-06-19T14:03:57Z

Maybe @ashishpd might know some history on that function/class and it's purpose in the platform?

Dnn.Platform/DNN Platform/Library/Framework/Reflections/TypeLocator.cs

Lines 69 to 124 in 6387dd1

    
           private bool CanScan(Assembly assembly) 
        
           { 
        
               string[] ignoreAssemblies = new string[] 
        
                                                   { 
        
                                                       "DotNetNuke.Authentication.Facebook", 
        
                                                       "DotNetNuke.Authentication.Google", 
        
                                                       "DotNetNuke.Authentication.LiveConnect", 
        
                                                       "DotNetNuke.Authentication.Twitter", 
        
                                                       "DotNetNuke.ASP2MenuNavigationProvider", 
        
                                                       "DotNetNuke.DNNDropDownNavigationProvider", 
        
                                                       "DotNetNuke.DNNMenuNavigationProvider", 
        
                                                       "DotNetNuke.DNNTreeNavigationProvider", 
        
                                                       "DotNetNuke.SolpartMenuNavigationProvider", 
        
                                                       "DotNetNuke.HttpModules", 
        
                                                       "DotNetNuke.Instrumentation", 
        
                                                       "DotNetNuke.Log4Net", 
        
                                                       "DotNetNuke.Modules.Groups", 
        
                                                       "DotNetNuke.Modules.Html", 
        
                                                       "DotNetNuke.Modules.HtmlEditorManager", 
        
                                                       "DotNetNuke.Modules.MobileManagement", 
        
                                                       "DotNetNuke.Modules.PreviewProfileManagement", 
        
                                                       "DotNetNuke.Modules.RazorHost", 
        
                                                       "DotNetNuke.Modules.Taxonomy", 
        
                                                       "DotNetNuke.Modules.UrlManagement", 
        
                                                       "DotNetNuke.RadEditorProvider", 
        
                                                       "DotNetNuke.Services.Syndication", 
        
                                                       "DotNetNuke.Web.Client", 
        
                                                       "DotNetNuke.Web.DDRMenu", 
        
                                                       "DotNetNuke.Web.Razor", 
        
                                                       "DotNetNuke.Web.Mvc", 
        
                                                       "DotNetNuke.WebControls", 
        
                                                       "DotNetNuke.WebUtility", 
        
                                                   }; 
        
               //First eliminate by "class" 
        
               var assemblyName = assembly.FullName.ToLowerInvariant(); 
        
               bool canScan = !(assemblyName.StartsWith("clientdependency.core") || assemblyName.StartsWith("countrylistbox") 
        
                   || assemblyName.StartsWith("icsharpcode") || assemblyName.StartsWith("fiftyone") 
        
                   || assemblyName.StartsWith("lucene") || assemblyName.StartsWith("microsoft") 
        
                   || assemblyName.StartsWith("newtonsoft") || assemblyName.StartsWith("petapoco") 
        
                   || assemblyName.StartsWith("sharpziplib") || assemblyName.StartsWith("system") 
        
                   || assemblyName.StartsWith("telerik") || assemblyName.StartsWith("webformsmvp") 
        
                   || assemblyName.StartsWith("webmatrix") || assemblyName.StartsWith("solpart") 
        
                   ); 
        
               if (canScan) 
        
               { 
        
                   //Next eliminate specific assemblies 
        
                   if (ignoreAssemblies.Any(ignoreAssembly => assemblyName == ignoreAssembly.ToLowerInvariant())) 
        
                   { 
        
                       canScan = false; 
        
                   } 
        
               } 
        
               return canScan; 
        
           }

mitchelsellers

Approved. @ohine is going to create a secondary PR to fix the identified issue

ohine · 2018-06-21T13:43:28Z

Created PR #2126 with the fix to the CanScan function.

iJungleboy added 2 commits June 12, 2018 09:07

Merge pull request #1 from dnnsoftware/development

8ab294a

sync from base

added fix for sharpziplib binary rename - fix #2095

3945269

iJungleboy requested review from bdukes and tpluscode June 12, 2018 20:29

This was referenced Jun 12, 2018

DNN-18077: add sharpziplib 0.81.0 install package for backward compatible. #2019

Closed

Dependencies to old SharpZipLib don't get resolved properly #2095

Closed

tpluscode reviewed Jun 13, 2018

View reviewed changes

bdukes requested changes Jun 13, 2018

View reviewed changes

fixes as requested in #2118

ae0fc75

moved all docs into code only

47bd5cb

tpluscode reviewed Jun 15, 2018

View reviewed changes

tpluscode approved these changes Jun 19, 2018

View reviewed changes

mitchelsellers approved these changes Jun 19, 2018

View reviewed changes

mitchelsellers merged commit eea53e5 into dnnsoftware:development Jun 19, 2018

ohine modified the milestone: 9.2.1 Jun 20, 2018

ohine mentioned this pull request Jun 21, 2018

add new sharpziplib assembly name to ignore list - Issue #2095 #2126

Closed

		@@ -0,0 +1,11 @@
		# Zip Services in DNN

		DNN uses SharpZipLib as the zip system of choice. Because of this, it is bundled with DNN.

Fix breaking dll rename of SharpZipLib - issue #2095 #2118

Fix breaking dll rename of SharpZipLib - issue #2095 #2118

Conversation

iJungleboy commented Jun 12, 2018

Summary

tpluscode left a comment

Choose a reason for hiding this comment

tpluscode Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdukes commented Jun 13, 2018

tpluscode commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iJungleboy Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iJungleboy Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iJungleboy commented Jun 14, 2018

iJungleboy commented Jun 14, 2018

ohine commented Jun 15, 2018 • edited Loading

iJungleboy commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdukes commented Jun 15, 2018

iJungleboy commented Jun 16, 2018

ohine commented Jun 17, 2018

iJungleboy commented Jun 19, 2018

mitchelsellers commented Jun 19, 2018

ohine commented Jun 19, 2018

mitchelsellers left a comment

Choose a reason for hiding this comment

ohine commented Jun 21, 2018

tpluscode Jun 13, 2018 •

edited

Loading

iJungleboy Jun 14, 2018 •

edited

Loading

iJungleboy Jun 14, 2018 •

edited

Loading

ohine commented Jun 15, 2018 •

edited

Loading