Implement OrderedDictionary<K, V> type
#3913
Conversation
flobernd
force-pushed
the
ordered_dictionary
branch
from
e61cc72 to
829202e
Compare
|
as reported in elastic/elasticsearch-java#932 |
flobernd
force-pushed
the
ordered_dictionary
branch
from
829202e to
94a2249
Compare
| @@ -773,6 +773,13 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma | |||
| } | |||
|
|
|||
| context.push(`Property '${prop.name}'`) | |||
|
|
|||
| if (prop.type.kind === 'dictionary_of' && prop.type.ordered) { | |||
| if (prop.name !== 'aggregations') { | |||
There was a problem hiding this comment.
This check only works for the "top-level" type of properties and does not check the type-name, but I think this is sufficient for now. WDYT?
There was a problem hiding this comment.
I guess this is to catch the many occurrences of aggregations as a property name?
It seems a bit brittle, but will ensure we can easily use OrderedDictionary for all aggregations property, which is where it should always be used.
... which makes me think that ideally we should have a validation that forbids Dictionary<string, Aggregation> to ensure we haven't forgotten anything.
There was a problem hiding this comment.
It seems a bit brittle, but will ensure we can easily use OrderedDictionary for all aggregations property, which is where it should always be used.
Correct!
... which makes me think that ideally we should have a validation that forbids Dictionary<string, Aggregation> to ensure we haven't forgotten anything.
I don't think there is a good way to detect this using a pattern. We have also cases like this:
Dictionary<string, ApiKeyAggregationContainer>where we define special containers to restrict the valid aggregations to a subset.
| @@ -88,6 +88,7 @@ export class DictionaryOf { | |||
| key: ValueOf | |||
| value: ValueOf | |||
| singleKey: boolean | |||
| ordered: boolean | |||
There was a problem hiding this comment.
Side comment: interesting case of a boolean property (singleKey) that evolves into an enum (singleKey, ordered, unordered) as ordered only makes sense when singleKey == false.
We can however keep it as two properties:
- some languages have their default dictionary keeping order (e.g. PHP, Ruby, JS) and their generators can safely ignore
orderedbecause it makes no difference to them, - some languages don't have ordered dictionaries (e.g. Go) and their generators will therefore also ignore
ordered.
There was a problem hiding this comment.
Yes, I was thinking about an enum, but I as well didn't want to break all existing tooling 😅
| @@ -500,7 +515,7 @@ export function modelEnumDeclaration (declaration: EnumDeclaration): model.Enum | |||
| } | |||
|
|
|||
| if (typeof tags.es_quirk === 'string') { | |||
| type.esQuirk = tags.es_quirk | |||
| type.esQuirk = tags.es_quirk.replace(/\r/g, '') | |||
There was a problem hiding this comment.
Yes 😵💫 I had applied this to most other places a while ago, but seems like I forgot about the quirks. This will ensure deterministic output regardless of being generated on *nix or Windows.
| @@ -773,6 +773,13 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma | |||
| } | |||
|
|
|||
| context.push(`Property '${prop.name}'`) | |||
|
|
|||
| if (prop.type.kind === 'dictionary_of' && prop.type.ordered) { | |||
| if (prop.name !== 'aggregations') { | |||
There was a problem hiding this comment.
I guess this is to catch the many occurrences of aggregations as a property name?
It seems a bit brittle, but will ensure we can easily use OrderedDictionary for all aggregations property, which is where it should always be used.
... which makes me think that ideally we should have a validation that forbids Dictionary<string, Aggregation> to ensure we haven't forgotten anything.
| @@ -20,3 +20,5 @@ | |||
| export class Dictionary<TKey, TValue> {} | |||
|
|
|||
| export class SingleKeyDictionary<TKey, TValue> {} | |||
|
|
|||
| export class OrderedDictionary<TKey, TValue> {} | |||
There was a problem hiding this comment.
We should restate the warning here, for people who will discover it in the API spec code.
There was a problem hiding this comment.
We also have another occurrence of this in elastic/elasticsearch-java#99 - Pivot.group_config should be ordered (and added in the validation):
Adds a new builtin dictionary type
OrderedDictionary<K, V>that can be used instead ofDictionary<K, V>when the order of items is important.Motivation
Some dictionaries require a fixed order (e.g.
aggregations), but currently we do not model this fact at all.This is quite special since the JSON spec does not make any guarantees about the order of properties in an object, but still Elasticsearch relies on that.
Closes: #3908
Related to: #3909