-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[utils] add util for converting between es and kibana types #11967
[utils] add util for converting between es and kibana types #11967
Conversation
a2e43b8
to
9ced8d5
Compare
9ced8d5
to
23b5e47
Compare
src/utils/kbn_field_types.js
Outdated
this.sortable = !!sortable; | ||
this.filterable = !!filterable; | ||
this.esTypes = Object.freeze(esTypes || []); | ||
Object.freeze(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.
Calling Object.freeze
in a constructor probably isn't a great pattern to get into the habit of using. If another class wanted to extend this one the freeze would prevent it from doing so. I suppose it's not particularly important in this instance though, it's unlikely anyone will want to extend this class.
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.
Freezing the field type instances is basically the whole reason this constructor exists, and it's only exported so it can be used in the tests. If subclassing was something we wanted to do then extensions would have to comply with the limitation (attach getters/setters/methods to the prototype) or update the KbnFieldType
class. I'm fine with that.
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.
I was just playing around with Object.freeze()
in another context and, unfortunately, I don't think it's a good idea to use it since it prevents angular from rendering it in an ng-repeat
, which requires adding a unique $$hashKey
to the object... I'll do something else.
}); | ||
|
||
it('returns undefined for invalid name', () => { | ||
expect(getKbnFieldType(Math.random())).to.be(undefined); |
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.
Adding randomness to a test like this doesn't seem like a good idea to me. What if it behaves differently if you pass in 0 vs 42? Also what makes the name invalid? Is it the fact that you're passing a non-string? If that's the case I would say 'returns undefined for non-string arguments'.
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.
I generally hesitate to use static strings for "invalid" values, so I wanted to use a little randomness and got lazy.
}); | ||
|
||
it('returns unknown for unknown es types', () => { | ||
expect(castEsToKbnFieldTypeName(Math.random())).to.be('unknown'); |
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.
Wouldn't it make more sense to test a string parameter here?
|
||
describe('castEsToKbnFieldType()', () => { | ||
it('returns the kbnFieldType instance that matches the esType', () => { | ||
expect(castEsToKbnFieldType('keyword')).to.be.a(KbnFieldType); |
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 is testing that the returned value is a KbnFieldType but not whether it is the correct KbnFieldType.
}); | ||
|
||
it('returns the unknown field type for unknown es types', () => { | ||
expect(castEsToKbnFieldType(Math.random())).to.be(getKbnFieldType('unknown')); |
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.
Same question about using a string param as above
@@ -21,7 +21,7 @@ uiModules | |||
const fieldTypesByLang = { | |||
painless: ['number', 'string', 'date', 'boolean'], | |||
expression: ['number'], | |||
default: _.keys(Private(IndexPatternsCastMappingTypeProvider).types.byType) | |||
default: getEsTypes() |
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 should be a list of Kibana types, not ES types.
if (!_.has(field, 'sortable')) field.sortable = type.sortable; | ||
if (!_.has(field, 'filterable')) field.filterable = type.filterable; | ||
return field; | ||
const kbnType = castEsToKbnFieldType(field.type); |
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 is always returning the unknown field type because field.type
already has the kibana type name.
Thanks for the review @Bargs, everything should be resolved. |
Object.freeze() keeps properties of an object from being redefined or removed, which we want for the kbnFieldTypes, but it also prevents them from being iterated by angular since angular needs to add a unique `$hashKey` property to each object. To keep the properties read only but allow extension KbnFieldType uses Object.defineProperties() instead.
a33c795
to
2b65f6c
Compare
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.
Just some minor nitpicks, but just feel free to skip them. LGTM
esTypes = [] | ||
} = options; | ||
|
||
Object.defineProperties(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.
A bit meh, but I understand why, so I'm good with it. If this is what we have to do to have immutable objects, this is what we do.
* @return {Array<string>} | ||
*/ | ||
export function getKbnTypeNames() { | ||
return KBN_FIELD_TYPES.map(type => type.name); |
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.
Almost feels like we should make KBN_FIELD_TYPES
an object keyed on the name. That would make this and getKbnFieldType
simpler. Not a big deal, though.
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.
I have a thing about storing lists of objects in anything other than an array 😷 . But I would be fine creating a secondary object like KBN_FIELD_TYPES_BY_NAME
or something.
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.
No worries. I'm fine with keeping this as-is.
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.
I agree it's fine as-is, but just to be philosophical about things, this is a scenario where a Map would be useful no?
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.
An extra map, for optimizing access by name, sure. But there are other types of access we might want to optimize for. And these optimizations are definitely on the nano-scale
src/fixtures/logstash_fields.js
Outdated
@@ -51,6 +53,10 @@ function stubbedLogstashFields() { | |||
scripted = !!script, | |||
} = metadata; | |||
|
|||
const type = (esType === 'conflict' || esType === 'unknown') |
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.
esType
can never be unknown
here.
Also, what is conflict
and why is it special-cased here? (instead of being handled within castEsToKbnFieldTypeName
)
(maybe just add a comment above this line that describes why we're special-casing these?)
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.
Correct, unknown
is not possible here. The conflict
type isn't from es, but is a special kbnFieldType, but we don't have any other way of representing here.
.when('GET', '/api/kibana/scripts/languages') | ||
.respond(['expression', 'painless']); | ||
getScriptedLangsResponse = $httpBackend.when('GET', '/api/kibana/scripts/languages'); | ||
getScriptedLangsResponse.respond(['expression', 'painless']); |
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.
My preference in beforeEach
: if it gets overridden in any test, it should always be handled within each test (so I only beforeEach
something if it concerns all the tests it covers, otherwise I find it just becomes more difficult to track the state of things)
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.
I tried to do something like this at first but it just spiraled out of control and changed way too much stuff.
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.
No worries, just skip it if it adds noise to this PR
|
||
describe('getKbnTypeNames()', () => { | ||
it('returns a list of all kbnFieldType names', () => { | ||
const esTypes = getKbnTypeNames(); |
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.
Maybe just .sort()
this and do an equals check?
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.
My intention was more about not duplicating the whole list of kbnType
names, but I'm fine with it. The list will basically never change
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 is perfect for snapshot testing 🎉 (you don't want to maintain the list yourself really, you just want to make sure you have something that fails if the list changes)
LGTM |
…11967) * [utils] add util for converting between es and kibana types * [utils/kbnFieldTypes] use random strings instead of numbers * [utils/kbnFieldTypes] ensure that castEsToKbnFieldType() returns correct instance * [utils/kbnFieldTypes] change getEsTypes() -> getKbnTypeNames() * [fieldEditor] update test to validate limited type support * [fieldEditor] unexpected scripted field langs should list all kbn types * [test/stubbedLogstashIndexPattern] fix kbnFieldType use * [utils/kbnFieldTypes] remove unused castEsToKbnFieldType() fn * [utils/kbnFieldTypes] don't use Object.freeze() Object.freeze() keeps properties of an object from being redefined or removed, which we want for the kbnFieldTypes, but it also prevents them from being iterated by angular since angular needs to add a unique `$hashKey` property to each object. To keep the properties read only but allow extension KbnFieldType uses Object.defineProperties() instead. * [fixtures/logstashFields] fix use of "unknown" and "conflict" types * [stubs/logstashFields] mention why "conflict" is special * [utils/kbnFieldTypes] check complete output of getKbnTypeNames() (cherry picked from commit c9afc8b)
…1967) (#11993) * [utils] add util for converting between es and kibana types (#11967) * [utils] add util for converting between es and kibana types * [utils/kbnFieldTypes] use random strings instead of numbers * [utils/kbnFieldTypes] ensure that castEsToKbnFieldType() returns correct instance * [utils/kbnFieldTypes] change getEsTypes() -> getKbnTypeNames() * [fieldEditor] update test to validate limited type support * [fieldEditor] unexpected scripted field langs should list all kbn types * [test/stubbedLogstashIndexPattern] fix kbnFieldType use * [utils/kbnFieldTypes] remove unused castEsToKbnFieldType() fn * [utils/kbnFieldTypes] don't use Object.freeze() Object.freeze() keeps properties of an object from being redefined or removed, which we want for the kbnFieldTypes, but it also prevents them from being iterated by angular since angular needs to add a unique `$hashKey` property to each object. To keep the properties read only but allow extension KbnFieldType uses Object.defineProperties() instead. * [fixtures/logstashFields] fix use of "unknown" and "conflict" types * [stubs/logstashFields] mention why "conflict" is special * [utils/kbnFieldTypes] check complete output of getKbnTypeNames() (cherry picked from commit c9afc8b) * [fieldEditor] remove test that only applies to 6.0+
…11967) * [utils] add util for converting between es and kibana types * [utils/kbnFieldTypes] use random strings instead of numbers * [utils/kbnFieldTypes] ensure that castEsToKbnFieldType() returns correct instance * [utils/kbnFieldTypes] change getEsTypes() -> getKbnTypeNames() * [fieldEditor] update test to validate limited type support * [fieldEditor] unexpected scripted field langs should list all kbn types * [test/stubbedLogstashIndexPattern] fix kbnFieldType use * [utils/kbnFieldTypes] remove unused castEsToKbnFieldType() fn * [utils/kbnFieldTypes] don't use Object.freeze() Object.freeze() keeps properties of an object from being redefined or removed, which we want for the kbnFieldTypes, but it also prevents them from being iterated by angular since angular needs to add a unique `$hashKey` property to each object. To keep the properties read only but allow extension KbnFieldType uses Object.defineProperties() instead. * [fixtures/logstashFields] fix use of "unknown" and "conflict" types * [stubs/logstashFields] mention why "conflict" is special * [utils/kbnFieldTypes] check complete output of getKbnTypeNames()
In order to share the same es<->kibana type conversion rules on the server and client this:
_cast_mapping_type
module from theui/index_patterns
intosrc/utils
getEsTypes()
, rather than portIndexedArray
to the serversrc/ui/public/index_patterns/_field_types.js