Skip to content

Commit

Permalink
Improve declaration of untagged unions to allow for generator optimiz…
Browse files Browse the repository at this point in the history
…ations (#2548)
  • Loading branch information
flobernd authored Jun 28, 2024
1 parent 42aaa69 commit 3f5c846
Show file tree
Hide file tree
Showing 16 changed files with 839 additions and 625 deletions.
9 changes: 9 additions & 0 deletions compiler-rs/clients_schema/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ pub enum ServerDefault {
pub enum Variants {
ExternalTag(ExternalTag),
InternalTag(InternalTag),
Untagged(Untagged),
Container(Container),
}

Expand All @@ -404,6 +405,13 @@ pub struct InternalTag {
pub default_tag: Option<String>,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Untagged {
#[serde(default)]
pub non_exhaustive: bool,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Container {
Expand Down Expand Up @@ -799,6 +807,7 @@ impl TypeAlias {
pub enum TypeAliasVariants {
ExternalTag(ExternalTag),
InternalTag(InternalTag),
Untagged(Untagged),
}

//------------------------------------------------------------------------------------------------------------
Expand Down
6 changes: 5 additions & 1 deletion compiler-rs/clients_schema_to_openapi/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.

use std::path::{Path, PathBuf};
use anyhow::bail;

use clap::{Parser, ValueEnum};
use clients_schema::{Availabilities, Visibility};
Expand Down Expand Up @@ -71,7 +72,10 @@ impl Cli {
std::fs::read_to_string(self.schema)?
};

let mut model: clients_schema::IndexedModel = serde_json::from_str(&json)?;
let mut model: clients_schema::IndexedModel = match serde_json::from_str(&json) {
Ok(indexed_model) => indexed_model,
Err(e) => bail!("cannot parse schema json: {}", e)
};

if let Some(flavor) = self.flavor {
if flavor != SchemaFlavor::All {
Expand Down
2 changes: 2 additions & 0 deletions compiler-rs/clients_schema_to_openapi/src/schemas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,8 @@ impl<'a> TypesAndComponents<'a> {
extensions: Default::default(),
});
}
Some(TypeAliasVariants::Untagged(_tag)) => {
}
};

Ok(schema)
Expand Down
2 changes: 1 addition & 1 deletion compiler-rs/openapi_to_clients_schema/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ fn generate_schema_kind_one_of(
let mut variants: Option<TypeAliasVariants> = None;

// TODO: do we want to allow untagged unions (those that are disambiguated by inspecting property names)?

if let Some(discriminator) = discriminator {
variants = Some(TypeAliasVariants::InternalTag(InternalTag {
default_tag: None,
Expand Down
14 changes: 11 additions & 3 deletions compiler/src/model/metamodel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export abstract class BaseType {
specLocation: string
}

export type Variants = ExternalTag | InternalTag | Container
export type Variants = ExternalTag | InternalTag | Container | Untagged

export class VariantBase {
/**
Expand All @@ -207,6 +207,11 @@ export class Container extends VariantBase {
kind: 'container'
}

export class Untagged extends VariantBase {
kind: 'untagged'
untypedVariant: Inherits
}

/**
* Inherits clause (aka extends or implements) for an interface or request
*/
Expand Down Expand Up @@ -364,8 +369,11 @@ export class TypeAlias extends BaseType {
type: ValueOf
/** generic parameters: either concrete types or open parameters from the enclosing type */
generics?: TypeName[]
/** Only applicable to `union_of` aliases: identify typed_key unions (external) and variant inventories (internal) */
variants?: InternalTag | ExternalTag
/**
* Only applicable to `union_of` aliases: identify typed_key unions (external), variant inventories (internal)
* and untagged variants
*/
variants?: InternalTag | ExternalTag | Untagged
}

// ------------------------------------------------------------------------------------------------
Expand Down
39 changes: 28 additions & 11 deletions compiler/src/model/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -536,8 +536,8 @@ export function modelTypeAlias (declaration: TypeAliasDeclaration): model.TypeAl
if (variants != null) {
assert(
declaration.getJsDocs(),
variants.kind === 'internal_tag' || variants.kind === 'external_tag',
'Type Aliases can only have internal or external variants'
variants.kind === 'internal_tag' || variants.kind === 'external_tag' || variants.kind === 'untagged',
'Type Aliases can only have internal, external or untagged variants'
)
typeAlias.variants = variants
}
Expand Down Expand Up @@ -1110,17 +1110,34 @@ export function parseVariantsTag (jsDoc: JSDoc[]): model.Variants | undefined {
}
}

assert(jsDoc, type === 'internal', `Bad variant type: ${type}`)

const pairs = parseKeyValues(jsDoc, values, 'tag', 'default')
assert(jsDoc, typeof pairs.tag === 'string', 'Internal variant requires a tag definition')
if (type === 'internal') {
const pairs = parseKeyValues(jsDoc, values, 'tag', 'default')
assert(jsDoc, typeof pairs.tag === 'string', 'Internal variant requires a tag definition')
return {
kind: 'internal_tag',
nonExhaustive: nonExhaustive,
tag: pairs.tag,
defaultTag: pairs.default
}
}

return {
kind: 'internal_tag',
nonExhaustive: nonExhaustive,
tag: pairs.tag,
defaultTag: pairs.default
if (type === 'untagged') {
const pairs = parseKeyValues(jsDoc, values, 'untyped')
assert(jsDoc, typeof pairs.untyped === 'string', 'Untagged variant requires an untyped definition')
const fqn = pairs.untyped.split('.')
return {
kind: 'untagged',
nonExhaustive: nonExhaustive,
untypedVariant: {
type: {
namespace: fqn.slice(0, fqn.length - 1).join('.'),
name: fqn[fqn.length - 1]
}
}
}
}

assert(jsDoc, false, `Bad variant type: ${type}`)
}

/**
Expand Down
70 changes: 68 additions & 2 deletions compiler/src/steps/validate-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -559,14 +559,14 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
if (typeDef.type.kind !== 'union_of') {
modelError('The "variants" tag only applies to unions')
} else {
validateTaggedUnion(typeDef.type, typeDef.variants)
validateTaggedUnion(typeDef.name, typeDef.type, typeDef.variants)
}
} else {
validateValueOf(typeDef.type, openGenerics)
}
}

function validateTaggedUnion (valueOf: model.UnionOf, variants: model.InternalTag | model.ExternalTag): void {
function validateTaggedUnion (parentName: TypeName, valueOf: model.UnionOf, variants: model.InternalTag | model.ExternalTag | model.Untagged): void {
if (variants.kind === 'external_tag') {
// All items must have a 'variant' attribute
const items = flattenUnionMembers(valueOf)
Expand Down Expand Up @@ -610,6 +610,72 @@ export default async function validateModel (apiModel: model.Model, restSpec: Ma
}

validateValueOf(valueOf, new Set())
} else if (variants.kind === 'untagged') {
if (fqn(parentName) !== '_types.query_dsl:DecayFunction' &&
fqn(parentName) !== '_types.query_dsl:DistanceFeatureQuery' &&
fqn(parentName) !== '_types.query_dsl:RangeQuery') {
throw new Error(`Please contact the devtools team before adding new untagged variant ${fqn(parentName)}`)
}

const untypedVariant = getTypeDef(variants.untypedVariant.type)
if (untypedVariant == null) {
modelError(`Type ${fqn(variants.untypedVariant.type)} not found`)
}

const items = flattenUnionMembers(valueOf)
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>())
const type = getTypeDef(item.type)
if (type == null) {
modelError(`Type ${fqn(item.type)} not found`)
} else {
if (type.kind !== 'interface') {
modelError(`Type ${fqn(item.type)} must be an interface to be used in an untagged union`)
continue
}

if (untypedVariant != null && fqn(item.type) === fqn(untypedVariant.name)) {
foundUntyped = true
}

if (type.inherits == null) {
modelError(`Type ${fqn(item.type)} must derive from a base type to be used in an untagged union`)
continue
}

baseTypes.add(fqn(type.inherits.type))

const baseType = getTypeDef(type.inherits.type)
if (baseType == null) {
modelError(`Type ${fqn(type.inherits.type)} not found`)
continue
}

if (baseType.kind !== 'interface') {
modelError(`Type ${fqn(type.inherits.type)} must be an interface to be used as the base class of another interface`)
continue
}

if (baseType.generics == null || baseType.generics.length === 0) {
modelError('The common base type of an untagged union must accept at least one generic type argument')
}
}
}
}

if (baseTypes.size !== 1) {
modelError('All items of an untagged union must derive from the same common base type')
}

if (!foundUntyped) {
modelError('The untyped variant of an untagged variant must be contained in the union items')
}
}
}

Expand Down
35 changes: 34 additions & 1 deletion docs/modeling-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ An annotation allows distinguishing these properties from container variants:

For example:

```
```ts
/**
* @variants container
*/
Expand All @@ -435,6 +435,39 @@ class AggregationContainer {
...
```
#### Untagged
The untagged variant is used for unions that can only be distinguished by the type of one or more fields.
> [!WARNING]
> This variant should only be used for legacy types and should otherwise be avoided as far as possible, as it leads to less optimal code generation in the client libraries.
The syntax is:
```ts
/** @variants untagged */
```
Untagged variants must exactly follow a defined pattern.
For example:
```ts
export class MyTypeBase<T1, T2, ...> { ... }

export class MyTypeUntyped extends MyTypeBase<UserDefinedValue> {}
export class MyTypeSpecialized1 extends MyTypeBase<int> {}
export class MyTypeSpecialized2 extends MyTypeBase<string> {}
export class MyTypeSpecialized3 extends MyTypeBase<bool> {}

/**
* @codegen_names untyped, mytype1, mytypet2, mytype3
* @variant untagged untyped=_types.MyTypeUntyped
*/
// Note: deserialization depends on value types
export type MyType = MyTypeUntyped | MyTypeSpecialized1 | MyTypeSpecialized2 | MyTypeSpecialized3
```
### Shortcut properties
In many places Elasticsearch accepts a property value to be either a complete data structure or a single value, that value being a shortcut for a property in the data structure.
Expand Down
Loading

0 comments on commit 3f5c846

Please sign in to comment.