Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a serialization issue with PermissionInfo #5943

Closed

Conversation

valadas
Copy link
Contributor

@valadas valadas commented Jan 29, 2024

This is a work in progress

This way appears to work for serializing as before the changes in #5841

But now I am bumping into an installation issue where Activator.CreateInstance inside CBO.CreateObject inside CBO.FillCollection (don't quote me on the exacts here) but it returns a null instead of an instance which causes this error during installation:

1/29/2024 07:12:44 [ERROR] DotNetNuke.Services.Upgrade.Upgrade Error: Value cannot be null.
Parameter name: source   at System.Linq.Enumerable.Where[TSource](IEnumerable`1 source, Func`2 predicate)
   at DotNetNuke.Security.Permissions.PermissionController.GetPermissionByCodeAndKeyEnumerable(String permissionCode, String permissionKey) in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Security\Permissions\PermissionController.cs:line 306
   at DotNetNuke.Security.Permissions.PermissionController.GetPermissionByCodeAndKey(String permissionCode, String permissionKey) in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Security\Permissions\PermissionController.cs:line 137
   at DotNetNuke.Entities.Modules.ModuleController.InitialModulePermission(ModuleInfo module, Int32 tabId, Int32 permissionType) in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Entities\Modules\ModuleController.cs:line 1138
   at DotNetNuke.Services.Upgrade.Upgrade.AddModuleToPage(TabInfo page, Int32 moduleDefId, String moduleTitle, String moduleIconFile, Boolean inheritPermissions, Boolean displayTitle, String paneName) in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Services\Upgrade\Upgrade.cs:line 290
   at DotNetNuke.Services.Upgrade.Upgrade.AddModuleToPage(TabInfo page, Int32 moduleDefId, String moduleTitle, String moduleIconFile, Boolean inheritPermissions) in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Services\Upgrade\Upgrade.cs:line 242
   at DotNetNuke.Services.Upgrade.Upgrade.AddModuleToPage(TabInfo page, Int32 moduleDefId, String moduleTitle, String moduleIconFile) in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Services\Upgrade\Upgrade.cs:line 2248
   at DotNetNuke.Services.Upgrade.Upgrade.UpgradeToVersion920() in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Services\Upgrade\Upgrade.cs:line 2750
   at DotNetNuke.Services.Upgrade.Upgrade.UpgradeApplication(String providerPath, Version version, Boolean writeFeedback) in C:\dnnvaladas\Dnn.Platform\DNN Platform\Library\Services\Upgrade\Upgrade.cs:line 1394

I am creating this WIP PR to probe your minds about some sort of pattern for the future ID renamings for Id (which will happen in many other places) I'd like to see what people think about simply having both casings during the deprecation phase and bypassing the CLSCompliance on only the wrong one.

I also needed some code to be here to refer to it in comments of other issues :)

@valadas valadas added this to the 9.13.3 milestone Jan 29, 2024
@valadas valadas marked this pull request as draft January 29, 2024 18:15
@bdukes
Copy link
Contributor

bdukes commented Jan 29, 2024

Allowing members which differ only by casing will break users of VB.Net (they should be able to cast to the interface to resolve the issue, but it would be breaking until they make that change).

@valadas
Copy link
Contributor Author

valadas commented Jan 29, 2024

@bdukes do you know if making the old one [CLSCompliant(false)] solves that ? My guess was that it would use the one without that attribute for VB but I did not try it, just assuming...

@bdukes
Copy link
Contributor

bdukes commented Jan 29, 2024

If you add a new property in an assembly and don't recompile the VB.Net assembly, it will ignore the new property (without regard for CLSCompliant attributes). Once compiled, it is no longer case insensitive.

However, any compile step will now cause build errors, whether there's a CLSCompliant attribute or not, specifically BC31429. This will be annoying for developers when updating which version they compile against. However, the major issue will be for code that is compiled at runtime (e.g. anything in .ascx files, App_Code, or codebehind files). These will stop working all of a sudden and take the site down. This seems less likely with permissions than with PortalId, but still a realistic scenario.

@valadas
Copy link
Contributor Author

valadas commented Jan 29, 2024

Thanks for your input @bdukes

I am not sure what the proper fix could be to this... #5930 does fix the issue at hand, just worried about others trying to serializing this class or any of its subclasses.

If I am not mistaken XmlSerializer.UnknownElement would only work when instantiating the serializer, so that would also only fix the issue for us.

Another idea that came to mind is to completely mark the class as obsolete instructing it will become internal in v11 and point consumers to use only the interface... Thoughts ?

@valadas
Copy link
Contributor Author

valadas commented Feb 13, 2024

I am closing this one, working on an analyzer...

@valadas valadas closed this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants