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

Resolve serialization issue #4788

Merged

Conversation

bdukes
Copy link
Contributor

@bdukes bdukes commented Aug 19, 2021

Summary

AuthenticationConfigBase is marked as Serializable but has an IServiceProvider property which cannot be serialized (it's a type defined outside of DNN and is not marked as Serializable). This causes an issue when using a caching provider that uses serialization, e.g. https://github.com/davidjrh/dnn.rediscachingprovider

This PR adds the NonSerialized attribute to the property and makes it retrieve the IServiceProvider dynamically (so that it always has a valid value, even after going through serialization).

Fixes #3592

Using a computed property instead of a property with a backing field
should be enough to avoid the serialization issue
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and should resolve the issue

@ggiordan
Copy link

@bdukes thank you so much!!

@ggiordan
Copy link

@bdukes I'm confused, I'm looking at the code for the file you changed in the main repo and it looks different that your branch. I see this:

 [Serializable]
    public abstract class AuthenticationConfigBase
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="AuthenticationConfigBase"/> class.
        /// </summary>
        public AuthenticationConfigBase()
        {
            this.DependencyProvider = Globals.DependencyProvider;
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="AuthenticationConfigBase"/> class.
        /// </summary>
        /// <param name="portalID"></param>
        protected AuthenticationConfigBase(int portalID)
            : this()
        {
            this.PortalID = portalID;
        }

        [Browsable(false)]
        public int PortalID { get; set; }

        /// <summary>
        /// Gets the Dependency Provider to resolve registered
        /// services with the container.
        /// </summary>
        /// <value>
        /// The Dependency Service.
        /// </value>
        protected IServiceProvider DependencyProvider { get; }
    }

The code you changed had an [unserializable] decoration. Also, the getter here is different.
Am I looking in the right place?
The reason I ask is that if this is a good fix, I need to patch my running system.

Thanks!!

Jeff

@bdukes
Copy link
Contributor Author

bdukes commented Aug 23, 2021

I had to remove the NotSerialized attribute because it only applies to fields, not properties. The change to the property is all that you should need.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@valadas valadas merged commit 396ab24 into dnnsoftware:develop Sep 9, 2021
@bdukes bdukes deleted the authentication-config-serialization branch September 20, 2021 14:19
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.

AuthenticationConfig RedisCachingProvider serialization error
4 participants