restore ISerializer ability, "safely"

[zachrybaker]

This will resolve issue 185

[stanleysmall-microsoft]

Thanks for this contribution! I think people will like being able to use custom serialization

[stanleysmall-microsoft]

@zachrybaker can you please agree to the CLA?

[zachrybaker]

@zachrybaker please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

[zachrybaker]

@microsoft-github-policy-service agree

[zachrybaker]

@stanleysmall-microsoft if you can recap what you’re looking for me at this point, I can try to put some attention on it over the weekend or something.

[zachrybaker]

Yes, the intention is to note how "THIS" instance of the data in redis is serialized, vs some other one. I have a totally different Blazor app also syncing this data. Zachry Baker

…

On Fri, Jan 12, 2024 at 12:19 PM Philo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/RedisSessionStateProvider/KeyGenerator.cs <#218 (comment)> : > private void GenerateKeys(string id, string app) { this.id = id; - DataKey = $"{{{app}_{id}}}_SessionStateItemCollection"; + DataKey = $"{{{app}_{id}}}_{SessionDataType}"; We should retain the "SessionStateItemCollection" portion of the key unless we need to eliminate it. What's your intention for the new SessionDataType? I assume to tag keys with a serializer identifier? — Reply to this email directly, view it on GitHub <#218 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACDSMQGDQSAUNCC6N6YTYLYOF5CDAVCNFSM6AAAAABA2HOYEOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMJYHAYDONBRGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[stanleysmall-microsoft]

@stanleysmall-microsoft if you can recap what you’re looking for me at this point, I can try to put some attention on it over the weekend or something.

@zachrybaker thanks for making this PR. I think your changes will improve the user experience of the package. I already made some changes to your branch which I think can be merged soon. The only questions I had were about publishSessionChanges and the renaming the session key. Maybe we could add these features in a separate PR? I'm just trying to understand the impact of these potential changes for now.

[zachrybaker]

Yes. I added the publish feature so that another app/process can subscribe to the update. My use case is that I am incrementally rewriting a large web forms app in Blazor and for most of that time period they will share session and profile data. Taking the strangler fig approach where both apps will coexist and the Blazor version will proxy unhandled requests to the old app. Since Blazor does not really have the HTTP request/response model but is a circuit - more of a current state machine, I needed a mechanism to synchronize profile and session between the two and this publish fits the bill perfectly.

…

On Tue, Jan 16, 2024 at 7:30 AM Stanley Small ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Shared/ProviderConfiguration.cs <#218 (comment)> : > @@ -87,6 +90,8 @@ private ProviderConfiguration(NameValueCollection config) // All below parameters are only fetched from web.config DatabaseId = GetIntSettings(config, "databaseId", 0); ApplicationName = GetStringSettings(config, "applicationName", null); + PublishSessionChanges = GetBoolSettings(config, "publishSessionChanges", false); @zachrybaker <https://github.com/zachrybaker> can you briefly describe the motivation behind adding the publishSessionChanges parameter? I don't think we should add it in this PR, but I'm curious about what this does. — Reply to this email directly, view it on GitHub <#218 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACDSMXR7ZRXBKKJWIM7IZLYOZ6HTAVCNFSM6AAAAABA2HOYEOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMRTGM3TONZZHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[zachrybaker]

@stanleysmall-microsoft Can you comment on what is going on with this PR and branch? AFAICT I submitted a PR to restore the ability to serialize to whatever format necessary, providing the means to do safe serialization. I also provided an option to publish the session so that subscribers (for example replacement apps using the Microsoft.SystemWebAdapters package) could interop and share session out of process. This is important for the Blazor use case, as session support in Blazor server is not there yet (and may never be). It seems you guys repurposed my branch to do maintenance work but axed my features and the whole purpose of the branch. Can you explain?

[frankyjquintero]

@stanleysmall-microsoft @philon-msft Can you comment on what is going on with this PR and branch?

[stanleysmall-microsoft]

Hi @frankyjquintero, I intend on merging and releasing this change soon. It is ready for review (in my opinion) and I am waiting for PR feedback.

[stanleysmall-microsoft]

After discussing with our team leadership, we’ve decided against merging this PR. Drivers for this decision include:

• This provider is on a retirement path because ASP.NET Core contains new built-in caching functionality that should be used instead. As the package approaches retirement, the priority is on maintaining stability and minimizing changes.
• As an MIT-licensed open source project, users are welcome to fork this code and modify it to add their own custom serialization as needed. In most cases, that approach will yield a simpler and more efficient solution than going through a generalized ISerializer.
• Users who prefer not to modify provider code can use the previous version 4.0.1 of this package, which does support custom serializers.

That said, we do appreciate the work that went into this contribution and the sense of community that it demonstrates. We look forward to working with you to evolve the caching story in the ASP.NET Core space. Recent developments there are simplifying cache integration while dramatically improving performance:

• New caching abstractions: https://devblogs.microsoft.com/dotnet/caching-abstraction-improvements-in-aspnetcore/
• Hybrid cache: https://www.youtube.com/watch?v=TDyZc11cJfA