Skip to content

Commit

Permalink
Fix operation parameter versioning validation (microsoft#2684)
Browse files Browse the repository at this point in the history 
Fixes microsoft#2680.

---------

Co-authored-by: Timothee Guerin <[email protected]>
  • Loading branch information
@tjprescott @timotheeguerin
tjprescott and timotheeguerin authored Nov 21, 2023
1 parent 7cdfe22 commit d2e72f6
Show file tree
Hide file tree
Showing 5 changed files with 296 additions and 25 deletions.
10 changes: 10 additions & 0 deletions common/changes/@typespec/compiler/versioning-OpParamsFix_2023-11-21-16-31.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/compiler",
"comment": "",
"type": "none"
}
],
"packageName": "@typespec/compiler"
}
10 changes: 10 additions & 0 deletions common/changes/@typespec/versioning/versioning-OpParamsFix_2023-11-21-16-31.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/versioning",
"comment": "",
"type": "none"
}
],
"packageName": "@typespec/versioning"
}
153 changes: 133 additions & 20 deletions packages/versioning/src/validate.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@ import {
getService,
getTypeName,
isTemplateInstance,
ModelProperty,
Namespace,
navigateProgram,
NoTarget,
Operation,
Program,
Type,
TypeNameOptions,
} from "@typespec/compiler";
import { reportDiagnostic } from "./lib.js";
import { Version } from "./types.js";
Expand All @@ -28,7 +31,7 @@ import {
export function $onValidate(program: Program) {
const namespaceDependencies = new Map<Namespace | undefined, Set<Namespace>>();

function addDependency(source: Namespace | undefined, target: Type | undefined) {
function addNamespaceDependency(source: Namespace | undefined, target: Type | undefined) {
if (!target || !("namespace" in target) || !target.namespace) {
return;
}
Expand All @@ -47,13 +50,15 @@ export function $onValidate(program: Program) {
if (isTemplateInstance(model)) {
return;
}
addDependency(model.namespace, model.sourceModel);
addDependency(model.namespace, model.baseModel);
addNamespaceDependency(model.namespace, model.sourceModel);
addNamespaceDependency(model.namespace, model.baseModel);
for (const prop of model.properties.values()) {
addDependency(model.namespace, prop.type);
addNamespaceDependency(model.namespace, prop.type);

// Validate model -> property have correct versioning
validateTargetVersionCompatible(program, model, prop, { isTargetADependent: true });
validateTargetVersionCompatible(program, model, prop, {
isTargetADependent: true,
});

// Validate model property -> type have correct versioning
const typeChangedFrom = getTypeChangedFrom(program, prop);
Expand All @@ -77,7 +82,7 @@ export function $onValidate(program: Program) {
return;
}
for (const variant of union.variants.values()) {
addDependency(union.namespace, variant.type);
addNamespaceDependency(union.namespace, variant.type);
}
validateVersionedPropertyNames(program, union);
},
Expand All @@ -88,16 +93,26 @@ export function $onValidate(program: Program) {
}

const namespace = op.namespace ?? op.interface?.namespace;
addDependency(namespace, op.sourceOperation);
addDependency(namespace, op.parameters);
addDependency(namespace, op.returnType);

addNamespaceDependency(namespace, op.sourceOperation);
addNamespaceDependency(namespace, op.returnType);
if (op.interface) {
validateTargetVersionCompatible(program, op.interface, op, { isTargetADependent: true });
}

for (const prop of op.parameters.properties.values()) {
validateReference(program, op, prop.type);
addNamespaceDependency(namespace, prop.type);

const typeChangedFrom = getTypeChangedFrom(program, prop);
if (typeChangedFrom !== undefined) {
validateMultiTypeReference(program, prop);
} else {
validateOperationParameter(program, op, prop);
}

// Validate model property type is correct when madeOptional
validateMadeOptional(program, prop);
}
validateVersionedPropertyNames(program, op.parameters);
validateReference(program, op, op.returnType);
},
interface: (iface) => {
Expand Down Expand Up @@ -188,7 +203,7 @@ function getAllVersions(p: Program, t: Type): Version[] | undefined {
/**
* Ensures that properties whose type has changed with versioning are valid.
*/
function validateMultiTypeReference(program: Program, source: Type) {
function validateMultiTypeReference(program: Program, source: Type, options?: TypeNameOptions) {
const versionTypeMap = getVersionedTypeMap(program, source);
if (versionTypeMap === undefined) return;
for (const [version, type] of versionTypeMap!) {
Expand All @@ -202,8 +217,8 @@ function validateMultiTypeReference(program: Program, source: Type) {
code: "incompatible-versioned-reference",
messageId: "doesNotExist",
format: {
sourceName: getTypeName(source),
targetName: getTypeName(type),
sourceName: getTypeName(source, options),
targetName: getTypeName(type, options),
version: prettyVersion(version),
},
target: source,
Expand Down Expand Up @@ -461,6 +476,82 @@ interface IncompatibleVersionValidateOptions {
isTargetADependent?: boolean;
}

/**
* Validate the target reference versioning is compatible with the source versioning.
* @param operation The operation being containing the parameter
* @param parameter The parameter used in the operation
*/
function validateOperationParameter(
program: Program,
operation: Operation,
parameter: ModelProperty
) {
const allVersions = getAllVersions(program, operation);
if (allVersions === undefined) return;
const alwaysAvailMap = new Map<string, Availability>();
allVersions.forEach((ver) => alwaysAvailMap.set(ver.name, Availability.Available));

const operationAvailability = getAvailabilityMapWithParentInfo(program, operation);
const paramAvailability = getAvailabilityMapWithParentInfo(program, parameter);
const paramTypeAvailability = getAvailabilityMapWithParentInfo(program, parameter.type);
const [paramTypeNamespace] = getVersions(program, parameter.type);
// everything is available in all versions
if (
operationAvailability === undefined &&
paramAvailability === undefined &&
paramTypeAvailability === undefined
) {
return;
}
// intrinstic types are always available
if (paramTypeNamespace === undefined) return;

// check if a parameter or parameter type is versioned but the operation is not
if (operationAvailability === undefined) {
if (paramAvailability !== undefined) {
reportDiagnostic(program, {
code: "incompatible-versioned-reference",
messageId: "default",
format: {
sourceName: getTypeName(operation),
targetName: getTypeName(parameter),
},
target: operation,
});
return;
} else if (paramTypeAvailability !== undefined) {
reportDiagnostic(program, {
code: "incompatible-versioned-reference",
messageId: "default",
format: {
sourceName: getTypeName(operation),
targetName: getTypeName(parameter.type),
},
target: operation,
});
return;
}
}

if (paramAvailability !== undefined) {
validateAvailabilityForContains(
program,
operationAvailability,
paramAvailability,
operation,
parameter
);
} else if (paramTypeAvailability !== undefined) {
validateAvailabilityForRef(
program,
operationAvailability,
paramTypeAvailability,
operation,
parameter.type
);
}
}

/**
* Validate the target reference versioning is compatible with the source versioning.
* This will also validate any template arguments used in the reference.
Expand Down Expand Up @@ -664,7 +755,9 @@ function validateAvailabilityForRef(
sourceAvail: Map<string, Availability> | undefined,
targetAvail: Map<string, Availability>,
source: Type,
target: Type
target: Type,
sourceOptions?: TypeNameOptions,
targetOptions?: TypeNameOptions
) {
// if source is unversioned and target is versioned
if (sourceAvail === undefined) {
Expand Down Expand Up @@ -717,6 +810,24 @@ function validateAvailabilityForRef(
target: source,
});
}
if (
[Availability.Unavailable].includes(sourceVal) &&
[Availability.Added, Availability.Available].includes(targetVal)
) {
const targetAddedOn = findAvailabilityAfterVersion(key, Availability.Added, sourceAvail);

reportDiagnostic(program, {
code: "incompatible-versioned-reference",
messageId: "addedAfter",
format: {
sourceName: getTypeName(target),
targetName: getTypeName(source),
sourceAddedOn: key,
targetAddedOn: targetAddedOn!,
},
target: target,
});
}
if (
[Availability.Removed].includes(sourceVal) &&
[Availability.Unavailable].includes(targetVal)
Expand All @@ -730,8 +841,8 @@ function validateAvailabilityForRef(
code: "incompatible-versioned-reference",
messageId: "removedBefore",
format: {
sourceName: getTypeName(source),
targetName: getTypeName(target),
sourceName: getTypeName(source, sourceOptions),
targetName: getTypeName(target, targetOptions),
sourceRemovedOn: key,
targetRemovedOn: targetRemovedOn!,
},
Expand All @@ -746,7 +857,9 @@ function validateAvailabilityForContains(
sourceAvail: Map<string, Availability> | undefined,
targetAvail: Map<string, Availability>,
source: Type,
target: Type
target: Type,
sourceOptions?: TypeNameOptions,
targetOptions?: TypeNameOptions
) {
if (!sourceAvail) return;

Expand All @@ -764,8 +877,8 @@ function validateAvailabilityForContains(
code: "incompatible-versioned-reference",
messageId: "dependentAddedAfter",
format: {
sourceName: getTypeName(source),
targetName: getTypeName(target),
sourceName: getTypeName(source, sourceOptions),
targetName: getTypeName(target, targetOptions),
sourceAddedOn: sourceAddedOn!,
targetAddedOn: key,
},
Expand Down
58 changes: 56 additions & 2 deletions packages/versioning/test/incompatible-versioning.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,19 @@ describe("versioning: validate incompatible references", () => {
});
});

it("emit diagnostic when unversioned op has a versioned parameter", async () => {
const diagnostics = await runner.diagnose(`
model Foo {}
op test(param: string, @added(Versions.v2) newParam: Foo): void;
`);
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' is referencing versioned type 'TestService.(anonymous model).newParam' but is not versioned itself.",
});
});

it("emit diagnostic when unversioned op based on a template has a versioned model as a parameter", async () => {
const diagnostics = await runner.diagnose(`
@added(Versions.v2)
Expand All @@ -133,7 +146,7 @@ describe("versioning: validate incompatible references", () => {
});
});

it("emit diagnostic when when op was added before parameter", async () => {
it("emit diagnostic when when op was added before parameter type", async () => {
const diagnostics = await runner.diagnose(`
@added(Versions.v2)
model Foo {}
Expand All @@ -148,7 +161,7 @@ describe("versioning: validate incompatible references", () => {
});
});

it("emit diagnostic when op based on a template was added before parameter", async () => {
it("emit diagnostic when op based on a template was added before parameter type", async () => {
const diagnostics = await runner.diagnose(`
@added(Versions.v2)
model Foo {}
Expand All @@ -164,6 +177,47 @@ describe("versioning: validate incompatible references", () => {
"'TestService.test' was added in version 'v1' but referencing type 'TestService.Foo' added in version 'v2'.",
});
});

it("emit diagnostic when when parameter was added before op", async () => {
const diagnostics = await runner.diagnose(`
model Foo {}
@added(Versions.v2)
op test(@added(Versions.v1) param: Foo): void;
`);
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' was added in version 'v2' but contains type 'TestService.(anonymous model).param' added in version 'v1'.",
});
});

it("emit diagnostic when type changed to types that don't exist", async () => {
const diagnostics = await runner.diagnose(`
@added(Versions.v3)
model Foo {}
@removed(Versions.v1)
model Doo {}
@added(Versions.v3)
op test(@typeChangedFrom(Versions.v2, Doo) param: Foo): void;
`);
expectDiagnostics(diagnostics, [
{
code: "@typespec/versioning/incompatible-versioned-reference",
severity: "error",
message:
"'TestService.(anonymous model).param' is referencing type 'TestService.Doo' which does not exist in version 'v1'.",
},
{
code: "@typespec/versioning/incompatible-versioned-reference",
severity: "error",
message:
"'TestService.(anonymous model).param' is referencing type 'TestService.Foo' which does not exist in version 'v2'.",
},
]);
});
});

describe("operation return type", () => {
Expand Down
Loading

0 comments on commit d2e72f6

Please sign in to comment.