Skip to content

hash parsing issue #124

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

Merged
merged 1 commit into from
Jun 30, 2022
Merged

hash parsing issue #124

merged 1 commit into from
Jun 30, 2022

Conversation

slorello89
Copy link
Member

Fixing issue with hash hydration caused by multiple prefixes matching the property name, this is caused by the way we handle serializing collections within hashes and embedded hashes within hashes, just need to check that the property is in what's reported back from Redis. Fixes #120

@slorello89 slorello89 requested a review from shacharPash June 7, 2022 17:28
@@ -348,14 +348,29 @@ private static string SendToJson(IDictionary<string, string> hash, Type t)

if (type == typeof(bool) || type == typeof(bool?))
{
if (!hash.ContainsKey(propertyName))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of writing this if 3 time, changing line 344 to: if (!hash.Any(x => x.Key == propertyName)) {continue;} will be more efficient.

If I understood you correctly, the line that I offered sepose to include the cases of the if's..

@slorello89 what do you think? am I missing somthing?

Copy link
Member Author

Choose a reason for hiding this comment

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

if only it were that easy, the thing is, you only check this if the underpinning type is not an enumerable and is not another object, the reason for this, if it's an array the property name should start with propertyname[, and if it's an object with sub-fields that we want to expand, it needs to start with propertyname., so really the only valid time to check this is these three scenarios, hence the slight duplication between the three.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds logical, I thought there was a nicer way to do it, but you know the code better than I do.

@shacharPash shacharPash merged commit d5efa84 into main Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retrieving a hash that was set without a property present throws exeption on SendToJson
2 participants