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

Fix Json deserializion of string containing hyphens to List<string> property #1094

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

ryangribble
Copy link
Contributor

Issue

As discussed in #1026, there is an issue with the Json parser in a specific edge case.

When deserialising a string property into a class where that property is declared as a List<string> (as is the case with the responses from the Enterprise SearchIndexing API), hyphens in the string are stripped during deserialization.

Json
{ "message":"User \"ryan-gribble\" was added to the indexing queue" }

Class
public class MessageList { public IReadOnlyList<string> Message { get; private set; } }

Result
"User \"ryangribble\" was added to the indexing queue"

Expected Result
"User \"ryan-gribble\" was added to the indexing queue"

Resolution

This line (introduced in #727) was identified as responsible for the removal of the hyphens in the input string. Code inspection indicates the line appears to be unnecessary for the actual Enum and Nullable Enum cases mentioned when the change was made, as they already had code to remove hyphens in their if blocks.

Unit tests were added to assert expected behaviour when deserializing a string attribute containing hyphens (and underscores for good measure) into string and <IReadOnlyList<string> properties.

After confirming the latter test failed, the offending line was removed and the tests then passed.

All other unit and integration tests were then also passed, indicating that no other known/tested functionality are impacted by the removal.

…ngs containing hyphens (and underscores for good measure) into a string and List<string> objects

- Fix failing List<string> test by removing the seemingly redundant line in the deserializer
@ryangribble
Copy link
Contributor Author

@shiftkey Linux build failure looks unrelated to me. Looks like a timeout in the octokit.tests-portable

@@ -90,7 +90,6 @@ public override object DeserializeObject(object value, Type type)
var jsonValue = value as JsonObject;
if (stringValue != null)
{
stringValue = stringValue.Replace("-", "");
Copy link
Member

Choose a reason for hiding this comment

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

The only reservation I had was that this value was being used elsewhere and some enum values depended on this, but then I saw those places were calling RemoveHyphenAndUnderscore and saw it had us covered:

static string RemoveHyphenAndUnderscore(string stringValue)
{
    // remove '-' from values coming in to be able to enum utf-8
    stringValue = stringValue.Replace("-", "");
    // remove '-' from values coming in to be able to enum EventInfoState names with underscores in them. Like "head_ref_deleted" 
    stringValue = stringValue.Replace("_", "");
    return stringValue;
}

@shiftkey
Copy link
Member

@ryangribble thanks for getting to the bottom of this, and for adding in those tests!

shiftkey added a commit that referenced this pull request Feb 11, 2016
Fix Json deserializion of string containing hyphens to List<string> property
@shiftkey shiftkey merged commit 3674670 into octokit:master Feb 11, 2016
@ryangribble ryangribble deleted the deserialize-hyphen branch February 11, 2016 05:02
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.

2 participants