splunk / splunk-sdk-csharp-pcl

Splunk's next generation C# SDK
https://dev.splunk.com/enterprise/docs/csharp
Apache License 2.0
64 stars 46 forks source link

Handle cases where we get an empty key for an s:dict entry #57

Closed itay closed 8 years ago

itay commented 8 years ago

When EAI sends us s:dict entries (with nested s:key nodes), we parse them as a dictionary in C#. However, it turns out the API can send us a case where the value for the s:key node is actually the empty string, which we do not expect and actually have a code contract to protect against.

In that case, we will change it from the empty string to the string called "empty" to be explicit about it. Since there can only be one empty key per level (otherwise it would be a serious API error), this is a reasonable workaround.

itay commented 8 years ago

An example bad XML snippet from the server:


      <s:key name="fieldMetadataStatic">
        <s:dict>
          <s:key name="">
            <s:dict>
              <s:key name="type">num</s:key>
            </s:dict>
          </s:key>
        </s:dict>
      </s:key>
      <s:key name="fieldMetadataEvents">
        <s:dict>
          <s:key name="">
            <s:dict>
              <s:key name="type">num</s:key>
            </s:dict>
          </s:key>
          <s:key name="EvaluatedField">
            <s:dict>
              <s:key name="type">num</s:key>
            </s:dict>
          </s:key>
        </s:dict>
      </s:key>
      <s:key name="fieldMetadataResults">
        <s:dict>
          <s:key name="">
            <s:dict>
              <s:key name="type">num</s:key>
            </s:dict>
          </s:key>
          <s:key name="EvaluatedField">
            <s:dict>
              <s:key name="type">num</s:key>
            </s:dict>
          </s:key>
        </s:dict>
      </s:key>
David-Noble-at-work commented 8 years ago

Looks good to me although I would prefer an alternative to "empty", perhaps "__empty" to minimize the risk of user-defined field name conflicts.

itay commented 8 years ago

FYI I will need to retarget this to develop. Will do it once all feedback is in.

shakeelmohamed commented 8 years ago

LGTM - I'd like to have a unit test in TestAtomFeed.cs that's basically the following (untested):

[Trait("unit-test", "Splunk.Client.AtomFeed")]
[Fact]
public async Task CanNormalizePropertyName()
{
    String goodName = AtomFeed.NormalizePropertyName("valid-name");
    Assert.Equal(result, "valid-name");
    String emptyName = AtomFeed.NormalizePropertyName("");
    Assert.Equal(emptyName, "empty");
}

edit: the empty string test can't be tested actually due to code contracts. We'll need to write tests around ParseDictionaryAsync

glennblock commented 8 years ago

Status here?

shakeelmohamed commented 8 years ago

Closed in favor of #60 against the develop branch