Skip to content

Commit

Permalink
Fix spec validator bugs (#3295)
Browse files Browse the repository at this point in the history 
* Fix open generics definition in type alias verification

* Add void builtin type

* A tagged union can have generic parameters

* Add binary builtin type

* Fix lint

---------

Co-authored-by: Quentin Pradet <[email protected]>
  • Loading branch information
@swallez @pquentin
swallez and pquentin authored Dec 13, 2024
1 parent fb25929 commit 9dd5b47
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 68 deletions.
36 changes: 16 additions & 20 deletions compiler/src/steps/validate-model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma

// Register builtin types
for (const name of [
'string', 'boolean', 'number', 'null'
'string', 'boolean', 'number', 'null', 'void', 'binary'
]) {
const typeName = {
namespace: '_builtins',
Expand Down Expand Up @@ -541,33 +541,29 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
}

function validateTypeAlias (typeDef: model.TypeAlias): void {
const openGenerics = new Set(typeDef.generics?.map(t => t.name))
const openGenerics = openGenericSet(typeDef)

if (typeDef.variants != null) {
if (typeDef.generics != null && typeDef.generics.length !== 0) {
modelError('A tagged union should not have generic parameters')
}

if (typeDef.type.kind !== 'union_of') {
modelError('The "variants" tag only applies to unions')
} else {
validateTaggedUnion(typeDef.name, typeDef.type, typeDef.variants)
validateTaggedUnion(typeDef.name, typeDef.type, typeDef.variants, openGenerics)
}
} else {
validateValueOf(typeDef.type, openGenerics)
}
}

function validateTaggedUnion (parentName: TypeName, valueOf: model.UnionOf, variants: model.InternalTag | model.ExternalTag | model.Untagged): void {
function validateTaggedUnion (parentName: TypeName, valueOf: model.UnionOf, variants: model.InternalTag | model.ExternalTag | model.Untagged, openGenerics: Set<string>): void {
if (variants.kind === 'external_tag') {
// All items must have a 'variant' attribute
const items = flattenUnionMembers(valueOf)
const items = flattenUnionMembers(valueOf, openGenerics)

for (const item of items) {
if (item.kind !== 'instance_of') {
modelError('Items of externally tagged unions must be types with a "variant_tag" annotation')
} else {
validateTypeRef(item.type, undefined, new Set<string>())
validateTypeRef(item.type, item.generics, openGenerics)
const type = getTypeDef(item.type)
if (type == null) {
modelError(`Type ${fqn(item.type)} not found`)
Expand All @@ -582,13 +578,13 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
}
} else if (variants.kind === 'internal_tag') {
const tagName = variants.tag
const items = flattenUnionMembers(valueOf)
const items = flattenUnionMembers(valueOf, openGenerics)

for (const item of items) {
if (item.kind !== 'instance_of') {
modelError('Items of internally tagged unions must be type references')
} else {
validateTypeRef(item.type, undefined, new Set<string>())
validateTypeRef(item.type, item.generics, openGenerics)
const type = getTypeDef(item.type)
if (type == null) {
modelError(`Type ${fqn(item.type)} not found`)
Expand All @@ -601,7 +597,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
}
}

validateValueOf(valueOf, new Set())
validateValueOf(valueOf, openGenerics)
} else if (variants.kind === 'untagged') {
if (fqn(parentName) !== '_types.query_dsl:DecayFunction' &&
fqn(parentName) !== '_types.query_dsl:DistanceFeatureQuery' &&
Expand All @@ -614,15 +610,15 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
modelError(`Type ${fqn(variants.untypedVariant)} not found`)
}

const items = flattenUnionMembers(valueOf)
const items = flattenUnionMembers(valueOf, openGenerics)
const baseTypes = new Set<string>()
let foundUntyped = false

for (const item of items) {
if (item.kind !== 'instance_of') {
modelError('Items of type untagged unions must be type references')
} else {
validateTypeRef(item.type, undefined, new Set<string>())
validateTypeRef(item.type, item.generics, openGenerics)
const type = getTypeDef(item.type)
if (type == null) {
modelError(`Type ${fqn(item.type)} not found`)
Expand Down Expand Up @@ -674,7 +670,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
// -----------------------------------------------------------------------------------------------
// Constituents of type definitions

function openGenericSet (typeDef: model.Request | model.Response | model.Interface): Set<string> {
function openGenericSet (typeDef: model.Request | model.Response | model.Interface | model.TypeAlias): Set<string> {
return new Set((typeDef.generics ?? []).map(name => fqn(name)))
}

Expand Down Expand Up @@ -879,7 +875,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
} else {
// tagged union: the discriminant tells us what to look for, check each member in isolation
assert(typeDef.type.kind === 'union_of', 'Variants are only allowed on union_of type aliases')
for (const item of flattenUnionMembers(typeDef.type)) {
for (const item of flattenUnionMembers(typeDef.type, new Set())) {
validateValueOfJsonEvents(item)
}

Expand All @@ -894,13 +890,13 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
}

/** Build the flattened item list of potentially nested unions (this is used for large unions) */
function flattenUnionMembers (union: model.UnionOf): model.ValueOf[] {
function flattenUnionMembers (union: model.UnionOf, openGenerics: Set<string>): model.ValueOf[] {
const allItems = new Array<model.ValueOf>()

function collectItems (items: model.ValueOf[]): void {
for (const item of items) {
if (item.kind !== 'instance_of') {
validateValueOf(item, new Set<string>())
validateValueOf(item, openGenerics)
allItems.push(item)
} else {
const itemType = getTypeDef(item.type)
Expand All @@ -910,7 +906,7 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
// Recurse in nested union
collectItems(itemType.type.items)
} else {
validateValueOf(item, new Set<string>())
validateValueOf(item, openGenerics)
allItems.push(item)
}
}
Expand Down
50 changes: 2 additions & 48 deletions output/schema/validation-errors.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,9 @@
],
"response": []
},
"async_search.get": {
"request": [],
"response": [
"type_alias definition _types:EpochTime / instance_of - No type definition for '_types.EpochTime:Unit'",
"type_alias definition _types.aggregations:Buckets / union_of / dictionary_of / instance_of - No type definition for '_types.aggregations.Buckets:TBucket'",
"type_alias definition _types.aggregations:Buckets / union_of / array_of / instance_of - No type definition for '_types.aggregations.Buckets:TBucket'",
"type_alias definition _spec_utils:Void / instance_of - No type definition for '_builtins:void'",
"type_alias definition _types:DurationValue / instance_of - No type definition for '_types.DurationValue:Unit'",
"type_alias definition _global.search._types:Suggest - A tagged union should not have generic parameters",
"type_alias definition _global.search._types:Suggest / instance_of / Generics / instance_of - No type definition for '_global.search._types.Suggest:TDocument'",
"type_alias definition _global.search._types:Suggest - Expected 1 generic parameters but got 0"
]
},
"async_search.submit": {
"request": [
"interface definition _types:QueryVectorBuilder - Property text_embedding is a single-variant and must be required",
"type_alias definition _spec_utils:PipeSeparatedFlags / union_of / instance_of - No type definition for '_spec_utils.PipeSeparatedFlags:T'"
"interface definition _types:QueryVectorBuilder - Property text_embedding is a single-variant and must be required"
],
"response": []
},
Expand All @@ -62,12 +48,6 @@
],
"response": []
},
"cat.allocation": {
"request": [],
"response": [
"type_alias definition _spec_utils:Stringified / union_of / instance_of - No type definition for '_spec_utils.Stringified:T'"
]
},
"cat.count": {
"request": [
"Request: query parameter 'master_timeout' does not exist in the json spec"
Expand Down Expand Up @@ -353,12 +333,6 @@
],
"response": []
},
"connector.update_error": {
"request": [
"type_alias definition _spec_utils:WithNullValue / union_of / instance_of - No type definition for '_spec_utils.WithNullValue:T'"
],
"response": []
},
"connector.update_features": {
"request": [
"Missing request & response"
Expand Down Expand Up @@ -415,12 +389,6 @@
],
"response": []
},
"esql.query": {
"request": [],
"response": [
"type_alias definition _types:EsqlColumns / instance_of - No type definition for '_builtins:binary'"
]
},
"features.get_features": {
"request": [
"Request: missing json spec query parameter 'master_timeout'"
Expand All @@ -445,12 +413,6 @@
],
"response": []
},
"fleet.msearch": {
"request": [],
"response": [
"type_alias definition _global.msearch:ResponseItem / union_of / instance_of / Generics / instance_of - No type definition for '_global.msearch.ResponseItem:TDocument'"
]
},
"fleet.post_secret": {
"request": [
"Missing request & response"
Expand Down Expand Up @@ -650,12 +612,6 @@
],
"response": []
},
"mget": {
"request": [],
"response": [
"type_alias definition _global.mget:ResponseItem / union_of / instance_of / Generics / instance_of - No type definition for '_global.mget.ResponseItem:TDocument'"
]
},
"ml.delete_trained_model": {
"request": [
"Request: missing json spec query parameter 'timeout'"
Expand Down Expand Up @@ -764,9 +720,7 @@
"Request: query parameter 'grid_agg' does not exist in the json spec",
"Request: missing json spec query parameter 'track_total_hits'"
],
"response": [
"type_alias definition _types:MapboxVectorTiles / instance_of - No type definition for '_builtins:binary'"
]
"response": []
},
"search_shards": {
"request": [
Expand Down

0 comments on commit 9dd5b47

Please sign in to comment.