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

Better JSON output scoping #6985

Merged
merged 1 commit into from Jul 24, 2014

Conversation

Projects
None yet
3 participants
@colings86
Copy link
Member

commented Jul 23, 2014

Before this change each aggregation had to output an object field with its name and write its JSON inside that object. This allowed for badly behaved aggregations which could write JSON content in the root of the 'aggs' object. this change move the writing of the aggregation name to a level above the aggregation itself, ensuring that aggregations can only write within there own scope in the JSON output.

Closes #7004

@colings86 colings86 added enhancement and removed v0.17.8 labels Jul 23, 2014

@colings86

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2014

This PR supersedes #6982 as it also fixes that bug. Maybe #6982 should go into 1.3.1 since its a true bug fix and this should go into 1.4 and 2.0 as its more of an enhancement?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2014

+1 on enforcing the serialization of the name.

Regarding the impl, I think it would be better to do the same thing that is done for the mappers: the base class has a toXContent method (that is not final because of some mappers but should be) that writes the name but then delegate to a doXContentBody method that sub-classes can extend.

This way, if we get other call sites of Aggregation.toXContent in the future, they won't need to care about serializing the agg name themselves.

+1 on fixing 1.3 as well but I think both changes should reference the same issue.

@jpountz jpountz removed the review label Jul 24, 2014

@colings86

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2014

Fixes #7004

@colings86 colings86 added review and removed enhancement labels Jul 24, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2014

LGTM

Aggregations: Better JSON output scoping
Before this change each aggregation had to output an object field with its name and write its JSON inside that object.  This allowed for badly behaved aggregations which could write JSON content in the root of the 'aggs' object.  this change move the writing of the aggregation name to a level above the aggregation itself, ensuring that aggregations can only write within there own scope in the JSON output.

Closes #7004

@colings86 colings86 merged commit fdf2bb9 into elastic:master Jul 24, 2014

@jpountz jpountz removed the review label Jul 29, 2014

@colings86 colings86 deleted the colings86:feature/aggsJSONImprovement branch Aug 13, 2014

@colings86 colings86 self-assigned this Aug 21, 2014

@clintongormley clintongormley changed the title Aggregations: Better JSON output scoping Better JSON output scoping Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.