-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add support for scripted fields #90
Conversation
@CGamesPlay I am horribly embarrassed that I for some reason didn't see this PR! I'm not super expert on ES, so I'll need someone else to weigh in on this. Maybe @nathancday or @worleydl have an opinon on how this was done. However, would it be possible to add a unit test demonstrating how this works? |
self.version = version; | ||
self.fieldsAttrName = fieldsAttrName; | ||
self.fieldsProperty = fieldsProperty; | ||
Doc.prototype = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this change w as made? Is htis a "better" way to do this? Are there other places through the codebase that should be like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was made so that the methods can be overridden in subclasses. This is a technique called "prototype inheritance", and yes, it should be used everywhere on all "classes" in JavaScript. I believe there are other places in the code base where we already store the methods on the prototype (instead of creating new methods on each instantiated object). However, if you want to "modernize" this, you should instead migrate the code to ES6 classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing this! I think I'd like to seperate what you've provided into two PR's if I can.. the first is for the support for scripted fields, and the second for resolving this "prototype inheritance" issue..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I don't have the bandwidth to write a unit test for this (we've already used it on our fork and are working on other projects now). Hopefully it isn't too difficult to add one, it's really just a matter of understanding that scripted fields occur in a different place in the result JSON from Elasticsearch than other fields do.
self.version = version; | ||
self.fieldsAttrName = fieldsAttrName; | ||
self.fieldsProperty = fieldsProperty; | ||
Doc.prototype = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was made so that the methods can be overridden in subclasses. This is a technique called "prototype inheritance", and yes, it should be used everywhere on all "classes" in JavaScript. I believe there are other places in the code base where we already store the methods on the prototype (instead of creating new methods on each instantiated object). However, if you want to "modernize" this, you should instead migrate the code to ES6 classes.
Humm.. rethinking my earlier comment and will take the refactorings that you did, because that appears to be in line with the dominant pattern in |
Co-authored-by: [email protected] <>
This change enables scripted fields to be queried when examining ElasticSearch results. It does so by merging the
_source
result property with thefields
result property for ElasticSearch documents.I'm not totally thrilled with the implementation, because the
fieldsAttrName
method isn't completely accurate. However, at least within splainer, this method isn't even used outside of thefieldsProperty
method. I'm open to feedback about the implementation choice, potentially even removingfieldsAttrName
if this is bothersome.