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

Stats API: do not convert arrays to objects #21053

Merged
merged 4 commits into from
Jul 23, 2018

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jul 20, 2018

This PR solves a problem where the toApiFieldNames method was converting arrays in the data to objects.

For example:

functions: [ 'max', 'min', 'avg' ]

would become

functions: { '0': 'max', '1': 'min', '2': 'avg' }

The changes in this PR handle arrays nested with object data as well.

@elasticmachine
Copy link
Contributor

💔 Build Failed

days_of_the_week: ['monday', 'tuesday', 'wednesday'],
});
});
});
Copy link
Contributor

@ycombinator ycombinator Jul 23, 2018

Choose a reason for hiding this comment

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

@tsullivan Could you write a test that uses an array of objects whose keys might need changing? So, for example:

const apiData = {
  daysOfTheWeek: [
    {
      dayName: 'monday',
      dayIndex: 1
    },
    {
      dayName: 'tuesday',
      dayIndex: 2
    },
    {
      dayName: 'wednesday',
      dayIndex: 3
    }
  ]
}

The expected result would be:

{
  days_of_the_week: [
    {
      day_name: 'monday',
      day_index: 1
    },
    {
      day_name: 'tuesday',
      day_index: 2
    },
    {
      day_name: 'wednesday',
      day_index: 3
    }
  ]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. That would not work with the code as it is. I stopped implementation on this before I started worrying about objects nested in arrays. But it is a fair ask.

Copy link
Member Author

Choose a reason for hiding this comment

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

};
}, {});
} else {
return apiData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the comments from @ycombinator about the test, it seems weird that we aren't doing any manipulation if it's an array since it seems reasonable it could be an array of objects that need to be manipulated?

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 (Array.isArray(apiData)) {
return apiData.map(getValueOrRecurse);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are returning from the if above, you don't necessarily need the else here. Somewhat related: https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#returnthrow-early-from-functions.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan merged commit fc5d329 into elastic:master Jul 23, 2018
@tsullivan tsullivan deleted the usage/stats-array-fix branch July 23, 2018 22:36
tsullivan added a commit to tsullivan/kibana that referenced this pull request Jul 23, 2018
* Stats API: do not convert arrays to objects

* handle nested arrays in toApiFieldNames

* add early return
tsullivan added a commit that referenced this pull request Jul 24, 2018
* Stats API: do not convert arrays to objects

* handle nested arrays in toApiFieldNames

* add early return
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants