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

Strip out dollar sign properties from msearches #5333

Merged
merged 3 commits into from Nov 5, 2015

Conversation

Projects
None yet
3 participants
@epixa
Member

epixa commented Nov 5, 2015

We store special properties for use in kibana that should not be sent
along to elasticsearch's msearch endpoint, and we denote them by
prefixing them with a dollar sign ($). This ensures they are properly
filtered out of the request body when serializing it to JSON.

Closes #5301

epixa added some commits Nov 4, 2015

Utility to parse $ properties while serializing json
toJson() will strip out any properties in the object that begin with a
dollar sign ($). If an optional custom toJson function is passed, then
the given object is first sent through that toJson function before being
reparsed and stringified with our dollar-sign-stripping behavior.
Strip out dollar sign properties from msearches
We store special properties for use in kibana that should not be sent
along to elasticsearch's msearch endpoint, and we denote them by
prefixing them with a dollar sign ($). This ensures they are properly
filtered out of the request body when serializing it to JSON.

@epixa epixa added the v4.3.0 label Nov 5, 2015

*
* The space argument is passed unaltered to the native stringify.
*/
export function toJson(object, jsonFn, space) {

This comment has been minimized.

@spalger

spalger Nov 5, 2015

Member

I don't understand the benefit of the jsonFn argument. If it's for performance reasons as the comment below implies it seems far less performant to encode, then decode, and then encode again rather than just encoding.

@spalger

spalger Nov 5, 2015

Member

I don't understand the benefit of the jsonFn argument. If it's for performance reasons as the comment below implies it seems far less performant to encode, then decode, and then encode again rather than just encoding.

This comment has been minimized.

@epixa

epixa Nov 5, 2015

Member

@spalger The vast majority of the time, we don't need the extra behaviors of angular.toJson, which is why this is an optional argument rather than a direct coupling to angular.toJson. In the event that you do want the extra serializing behaviors of angular.toJson, then you can pass it here.

Ideally we'd just compose our own replacer function with the replacer function that angular uses, but angular doesn't expose it's toJsonReplacer function, so we have no way of accessing it.

@epixa

epixa Nov 5, 2015

Member

@spalger The vast majority of the time, we don't need the extra behaviors of angular.toJson, which is why this is an optional argument rather than a direct coupling to angular.toJson. In the event that you do want the extra serializing behaviors of angular.toJson, then you can pass it here.

Ideally we'd just compose our own replacer function with the replacer function that angular uses, but angular doesn't expose it's toJsonReplacer function, so we have no way of accessing it.

This comment has been minimized.

@spalger

spalger Nov 5, 2015

Member

What behavior does angular.toJson provide that we don't?

@spalger

spalger Nov 5, 2015

Member

What behavior does angular.toJson provide that we don't?

This comment has been minimized.

@spalger

spalger Nov 5, 2015

Member

Oh, I see there are some edge cases that angular covers... Interesting https://github.com/angular/angular.js/blob/v1.4.x/src/Angular.js#L1136-L1150

@spalger

spalger Nov 5, 2015

Member

Oh, I see there are some edge cases that angular covers... Interesting https://github.com/angular/angular.js/blob/v1.4.x/src/Angular.js#L1136-L1150

This comment has been minimized.

@epixa

epixa Nov 5, 2015

Member

The comment below is not about performance, it's about the special handling JSON.stringify has for avoiding infinite loops when encountering self-referencing objects.

@epixa

epixa Nov 5, 2015

Member

The comment below is not about performance, it's about the special handling JSON.stringify has for avoiding infinite loops when encountering self-referencing objects.

This comment has been minimized.

@spalger

spalger Nov 5, 2015

Member

Ah, I think I get it. Are you posing this as an alternative to iterating the object yourself? I guess I wasn't considering that option. Either way 👍

@spalger

spalger Nov 5, 2015

Member

Ah, I think I get it. Are you posing this as an alternative to iterating the object yourself? I guess I wasn't considering that option. Either way 👍

This comment has been minimized.

@epixa

epixa Nov 5, 2015

Member

Yep. I had originally implemented omit$ and deepOmit$ functions that object would pass through before being dropped into angular.json directly, but I hit the infinite recursion wall when testing on some random objects throughout kibana, so I scrapped them in favor of relying on the native construct to catch that sort of thing.

@epixa

epixa Nov 5, 2015

Member

Yep. I had originally implemented omit$ and deepOmit$ functions that object would pass through before being dropped into angular.json directly, but I hit the infinite recursion wall when testing on some random objects throughout kibana, so I scrapped them in favor of relying on the native construct to catch that sort of thing.

@spalger

This comment has been minimized.

Show comment
Hide comment
@spalger

spalger Nov 5, 2015

Member

LGTM

Member

spalger commented Nov 5, 2015

LGTM

1 similar comment
@jbudz

This comment has been minimized.

Show comment
Hide comment
@jbudz

jbudz Nov 5, 2015

Contributor

LGTM

Contributor

jbudz commented Nov 5, 2015

LGTM

epixa added a commit that referenced this pull request Nov 5, 2015

Merge pull request #5333 from epixa/5301-omit-dollar-props-toJson
Strip out dollar sign properties from msearches

@epixa epixa merged commit 3c7e486 into elastic:master Nov 5, 2015

3 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Merged build finished.
Details

@epixa epixa deleted the epixa:5301-omit-dollar-props-toJson branch Nov 5, 2015

epixa added a commit that referenced this pull request Nov 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment