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

${aspnet-session} - added ValueType to handle integer session values #570

Merged
merged 4 commits into from
Jun 22, 2020

Conversation

304NotModified
Copy link
Member

@304NotModified 304NotModified commented May 27, 2020

see https://stackoverflow.com/questions/62054400/can-the-nlog-aspnet-sessionvariable-intid-renderer-extract-display-integers-f/62054553#62054553

I think the only solution here is to add a type option to the renderer. What do you think @snakefoot ?

(If it's 4 bytes long, we only know it could be an int)

PS: these are the extensions in ASP.NET Core:

namespace Microsoft.AspNetCore.Http
{
    public static class SessionExtensions
    {
        public static void SetInt32(this ISession session, string key, int value)
        {
            var bytes = new byte[]
            {
                (byte)(value >> 24),
                (byte)(0xFF & (value >> 16)),
                (byte)(0xFF & (value >> 8)),
                (byte)(0xFF & value)
            };
            session.Set(key, bytes);
        }

        public static int? GetInt32(this ISession session, string key)
        {
            var data = session.Get(key);
            if (data == null || data.Length < 4)
            {
                return null;
            }
            return data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3];
        }

        public static void SetString(this ISession session, string key, string value)
        {
            session.Set(key, Encoding.UTF8.GetBytes(value));
        }

        public static string GetString(this ISession session, string key)
        {
            var data = session.Get(key);
            if (data == null)
            {
                return null;
            }
            return Encoding.UTF8.GetString(data);
        }

        public static byte[] Get(this ISession session, string key)
        {
            byte[] value = null;
            session.TryGetValue(key, out value);
            return value;
        }
    }
}

@snakefoot
Copy link
Contributor

snakefoot commented May 28, 2020

Instead of having a generic property-type, Then just have Int32 / string as enum-values. But yes a ValueType-property sounds fine.

@snakefoot
Copy link
Contributor

Strange that Microsoft have not added some kind of byte-order-mark to signal whether byte-array is an integer or string.

@304NotModified
Copy link
Member Author

Or just an int which would be utf8 compatible. Yes, less efficient. This int "encoding" is just unsafe stuff

@snakefoot
Copy link
Contributor

Yes also think it would be nicer that the SetInt32-extension-method, just converted integer into utf8-byte-array without doing string-allocation first. Not sure people using session-state-objects care much about 4 or 10 bytes.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 21, 2020
@304NotModified 304NotModified changed the title Handle int for session value (ASP.NET Core) - Currently test only Handle int for session value (ASP.NET Core) Jun 21, 2020
@304NotModified 304NotModified changed the title Handle int for session value (ASP.NET Core) Handle int for session value Jun 21, 2020
@304NotModified 304NotModified marked this pull request as ready for review June 21, 2020 21:43
@304NotModified 304NotModified requested a review from snakefoot June 21, 2020 21:43
Copy link
Contributor

@snakefoot snakefoot left a comment

Choose a reason for hiding this comment

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

Looks great

/// <summary>
/// The type of a value
/// </summary>
public enum SessionValueType
Copy link
Contributor

Choose a reason for hiding this comment

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

Like the name

@@ -62,7 +62,7 @@ internal static HttpResponse TryGetResponse(this HttpContext context)
}
#endif

#if ASP_NET_CORE
#if ASP_NET_CORE1 || ASP_NET_CORE2
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea since ASP_NET_CORE3 includes entire framework

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

87.5% 87.5% Coverage
0.0% 0.0% Duplication

@304NotModified 304NotModified merged commit 99d93f1 into master Jun 22, 2020
@304NotModified 304NotModified deleted the aspnetcore-session-int branch June 22, 2020 08:58
@304NotModified 304NotModified added this to the 4.9.3 milestone Jun 22, 2020
@304NotModified 304NotModified mentioned this pull request Jun 22, 2020
1 task
@304NotModified
Copy link
Member Author

@304NotModified 304NotModified changed the title Handle int for session value Handle int for session value - added ValueType Aug 1, 2020
@304NotModified 304NotModified changed the title Handle int for session value - added ValueType ${aspnet-session} - added ValueType to handle integer session values Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants