Skip to content
This repository was archived by the owner on Apr 4, 2023. It is now read-only.

Conversation

@itay
Copy link
Contributor

@itay itay commented Jun 8, 2016

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.

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
Copy link
Contributor Author

itay commented Jun 8, 2016

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
Copy link

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
Copy link
Contributor Author

itay commented Jun 8, 2016

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

@shakeelmohamed
Copy link
Contributor

shakeelmohamed commented Jun 8, 2016

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
Copy link
Contributor

Status here?

@shakeelmohamed
Copy link
Contributor

Closed in favor of #60 against the develop branch

@shakeelmohamed shakeelmohamed deleted the bugfix/empty-key-eval branch September 19, 2016 17:14
shakeelmohamed pushed a commit that referenced this pull request Oct 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants