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.

Sign up for GitHub

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

Implement OrderedDictionary<K, V> type #3913

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+525 −13
Open

Implement OrderedDictionary<K, V> type #3913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/src/model/metamodel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export class DictionaryOf {
key: ValueOf
value: ValueOf
singleKey: boolean
ordered: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ordered because it makes no difference to them,
  • some languages don't have ordered dictionaries (e.g. Go) and their generators will therefore also ignore ordered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was thinking about an enum, but I as well didn't want to break all existing tooling 😅

}

/**
Expand Down
23 changes: 19 additions & 4 deletions compiler/src/model/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ export function modelType (node: Node): model.ValueOf {
kind: 'dictionary_of',
key,
value,
singleKey: false
singleKey: false,
ordered: false
}
return type
}
Expand All @@ -294,7 +295,21 @@ export function modelType (node: Node): model.ValueOf {
kind: 'dictionary_of',
key,
value,
singleKey: true
singleKey: true,
ordered: false
}
return type
}

case 'OrderedDictionary': {
assert(node, node.getTypeArguments().length === 2, 'A OrderedDictionary must have two arguments')
const [key, value] = node.getTypeArguments().map(node => modelType(node))
const type: model.DictionaryOf = {
kind: 'dictionary_of',
key,
value,
singleKey: false,
ordered: true
}
return type
}
Expand Down Expand Up @@ -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, '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Window-ism?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return type
Expand Down Expand Up @@ -892,7 +907,7 @@ function hoistPropertyAnnotations (property: model.Property, jsDocs: JSDoc[]): v
assert(jsDocs, value === 'container_property', `Unknown 'variant' value '${value}' on property ${property.name}`)
property.containerProperty = true
} else if (tag === 'es_quirk') {
property.esQuirk = value
property.esQuirk = value.replace(/\r/g, '')
} else {
assert(jsDocs, false, `Unhandled tag: '${tag}' with value: '${value}' on property ${property.name}`)
}
Expand Down
19 changes: 13 additions & 6 deletions compiler/src/steps/validate-model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,17 +396,17 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
const inheritedProps = inheritedProperties(typeDef)

context.push('path')
validateProperties(typeDef.path, openGenerics, inheritedProps)
validateProperties(fqn(typeDef.name), typeDef.path, openGenerics, inheritedProps)
context.pop()

context.push('query')
validateProperties(typeDef.query, openGenerics, inheritedProps)
validateProperties(fqn(typeDef.name), typeDef.query, openGenerics, inheritedProps)
context.pop()

context.push('body')
switch (typeDef.body.kind) {
case 'properties':
validateProperties(typeDef.body.properties, openGenerics, inheritedProps)
validateProperties(fqn(typeDef.name), typeDef.body.properties, openGenerics, inheritedProps)
break
case 'value':
validateValueOf(typeDef.body.value, openGenerics)
Expand All @@ -433,7 +433,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma

switch (typeDef.body.kind) {
case 'properties':
validateProperties(typeDef.body.properties, openGenerics, inheritedProperties(typeDef))
validateProperties(fqn(typeDef.name), typeDef.body.properties, openGenerics, inheritedProperties(typeDef))
break
case 'value':
validateValueOf(typeDef.body.value, openGenerics)
Expand Down Expand Up @@ -507,7 +507,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma

validateInherits(typeDef.inherits, openGenerics)
validateBehaviors(typeDef, openGenerics)
validateProperties(typeDef.properties, openGenerics, inheritedProperties(typeDef))
validateProperties(fqn(typeDef.name), typeDef.properties, openGenerics, inheritedProperties(typeDef))

if (typeDef.variants?.kind === 'container') {
const variants = typeDef.properties.filter(prop => !(prop.containerProperty ?? false))
Expand Down Expand Up @@ -747,7 +747,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
return false
}

function validateProperties (props: model.Property[], openGenerics: Set<string>, inheritedProperties: Set<string>): void {
function validateProperties (type: string, props: model.Property[], openGenerics: Set<string>, inheritedProperties: Set<string>): void {
const allIdentifiers = new Set<string>()
const allNames = new Set<string>()

Expand All @@ -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') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

modelError(`OrderedDictionary can not be used for property '${prop.name}' on type '${type}'.`)
}
}

validateValueOf(prop.type, openGenerics)
validateValueOfJsonEvents(prop.type)
context.pop()
Expand Down
3 changes: 2 additions & 1 deletion compiler/src/transform/expand-generics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,8 @@ export function expandGenerics (inputModel: Model, config?: ExpansionConfig): Mo
kind: 'dictionary_of',
key: expandValueOf(value.key, mappings),
value: expandValueOf(value.value, mappings),
singleKey: value.singleKey
singleKey: value.singleKey,
ordered: value.ordered
}

case 'instance_of': {
Expand Down
16 changes: 15 additions & 1 deletion docs/modeling-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,24 @@ For example:
```json
{
"property1": "type",
"property2": "other-type",
"property2": "other-type"
}
```

### OrderedDictionary

Represents a dynamic key value map that preserves the order of items:

```ts
property: OrderedDictionary<string, TypeDefinition>
```

The JSON specification and most dictionary implementations do not make guarantees about item ordering.
`OrderedDictionary` can be used to express this fact in the Elasticsearch specification.

> [!WARNING]
> This type should only be used for legacy types and should otherwise be avoided as far as possible.

### SingleKeyDictionary

Represents a dynamic key value map with a single top level key:
Expand Down
Loading
Loading