-
Notifications
You must be signed in to change notification settings - Fork 65
Case insensitive session keys JSON serializer #540
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
Conversation
@dotnet-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix and the tests. I've left a request to retain the original behavior but allow you to override it.
@@ -14,7 +14,7 @@ public class JsonSessionSerializerOptions | |||
/// <summary> | |||
/// Gets the mapping of known session keys to types | |||
/// </summary> | |||
public IDictionary<string, Type> KnownKeys { get; } = new Dictionary<string, Type>(); | |||
public IDictionary<string, Type> KnownKeys { get; } = new Dictionary<string, Type>(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a potentially breaking change and I'd prefer it to be done via an additional option:
public class JsonSessionSerializerOptions
{
+ public IEqualityComparer<string> KeyComparer { get; set; } = StringComparer.Ordinal;
}
It should also check if the KnownKeys
has been created to throw if the setter is called. I'd also be ok if changing the comparer creates a new dictionary with the new comparer and the contents of the previous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, if you can show that the session keys on .NET Framework were case insensitive, then we can change the default since we're already doing a major change for v2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just need a few tweaks to a test
} | ||
|
||
[Fact] | ||
public void ChangeSessionKeysComparer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test isn't really testing that the comparer is used, just that the keys are still there. Can you verify that the new comparer is use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. It checks the key case insensitively though? The key (key2
) upper case is verified it does not, and after the change that it does exists (it was registered lower case). That wouldn't work if it if the comparer didn't change?
How would you like me to test? Check the Dictionary
itself?
if (options.KnownKeys is Dictionary<string, Type> dict)
{
Assert.Equal(dict.Comparer, StringComparer.OrdinalIgnoreCase);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I guess I read the change too quickly. lgtm :)
} | ||
|
||
[Fact] | ||
public void ChangeSessionKeysComparer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I guess I read the change too quickly. lgtm :)
PR Title
Case insensitive session keys for the JSON serializer.
Addresses #536