Skip to content

Commit

Permalink
Fix response examples with union not showing up correctly (#4046)
Browse files Browse the repository at this point in the history
fix #4033

A few issues:
- Example being empty 
- Example duplicating for all responses instead of mapping to the right
one

---------

Co-authored-by: Christopher Radek <14189820+chrisradek@users.noreply.github.com>
  • Loading branch information
timotheeguerin and chrisradek committed Aug 5, 2024
1 parent e8e816e commit 7b5d263
Show file tree
Hide file tree
Showing 17 changed files with 610 additions and 212 deletions.
8 changes: 8 additions & 0 deletions .chronus/changes/fix-response-examples-2024-6-29-22-38-45.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
changeKind: fix
packages:
- "@typespec/compiler"
---

Fix type comparison of literal and scalar when in projection context
7 changes: 7 additions & 0 deletions .chronus/changes/fix-response-examples-2024-6-30-15-26-28.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: fix
packages:
- "@typespec/openapi3"
---

Fix issue where operation example would produce an empty object when `@body`/`@bodyRoot` was used
8 changes: 8 additions & 0 deletions .chronus/changes/fix-response-examples-2024-6-30-15-59-29.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
changeKind: fix
packages:
- "@typespec/openapi3"
---

Fix operation response body examples showing up for each response.
8 changes: 8 additions & 0 deletions .chronus/changes/fix-response-examples-2024-6-30-22-22-9.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
changeKind: feature
packages:
- "@typespec/http"
---

API: Expose `properties: HttpProperty[]` on operation parameter and response which reference all the properties of interest(Body, statusCode, contentType, implicitBodyProperty, etc.)
4 changes: 2 additions & 2 deletions packages/compiler/src/core/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7874,7 +7874,7 @@ export function createChecker(program: Program): Checker {

function isNumericAssignableToNumericScalar(source: Numeric, target: Scalar) {
// if the target does not derive from numeric, then it can't be assigned a numeric literal
if (!areScalarsRelated(target, getStdType("numeric"))) {
if (!areScalarsRelated((target.projectionBase as any) ?? target, getStdType("numeric"))) {
return false;
}

Expand Down Expand Up @@ -7902,7 +7902,7 @@ export function createChecker(program: Program): Checker {
}

function isStringLiteralRelatedTo(source: StringLiteral | StringTemplate, target: Scalar) {
if (!areScalarsRelated(target, getStdType("string"))) {
if (!areScalarsRelated((target.projectionBase as any) ?? target, getStdType("string"))) {
return false;
}
if (source.kind === "StringTemplate") {
Expand Down
85 changes: 46 additions & 39 deletions packages/http/src/http-property.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
type ModelProperty,
type Program,
} from "@typespec/compiler";
import { Queue } from "@typespec/compiler/utils";
import {
getHeaderFieldOptions,
getPathParamOptions,
Expand All @@ -36,6 +35,8 @@ export type HttpProperty =

export interface HttpPropertyBase {
readonly property: ModelProperty;
/** Path from the root of the operation parameters/returnType to the property. */
readonly path: (string | number)[];
}

export interface HeaderProperty extends HttpPropertyBase {
Expand Down Expand Up @@ -78,14 +79,17 @@ export interface GetHttpPropertyOptions {
/**
* Find the type of a property in a model
*/
export function getHttpProperty(
function getHttpProperty(
program: Program,
property: ModelProperty,
path: (string | number)[],
options: GetHttpPropertyOptions = {}
): [HttpProperty, readonly Diagnostic[]] {
const diagnostics: Diagnostic[] = [];
function createResult<T extends HttpProperty>(opts: T): [T, readonly Diagnostic[]] {
return [{ ...opts, property } as any, diagnostics];
function createResult<T extends Omit<HttpProperty, "path" | "property">>(
opts: T
): [HttpProperty & T, readonly Diagnostic[]] {
return [{ ...opts, property, path } as any, diagnostics];
}

const annotations = {
Expand All @@ -106,10 +110,9 @@ export function getHttpProperty(
name: property.name,
type: "path",
},
property,
});
}
return [{ kind: "bodyProperty", property }, []];
return createResult({ kind: "bodyProperty" });
} else if (defined.length > 1) {
diagnostics.push(
createDiagnostic({
Expand All @@ -122,24 +125,24 @@ export function getHttpProperty(

if (annotations.header) {
if (annotations.header.name.toLowerCase() === "content-type") {
return createResult({ kind: "contentType", property });
return createResult({ kind: "contentType" });
} else {
return createResult({ kind: "header", options: annotations.header, property });
return createResult({ kind: "header", options: annotations.header });
}
} else if (annotations.query) {
return createResult({ kind: "query", options: annotations.query, property });
return createResult({ kind: "query", options: annotations.query });
} else if (annotations.path) {
return createResult({ kind: "path", options: annotations.path, property });
return createResult({ kind: "path", options: annotations.path });
} else if (annotations.statusCode) {
return createResult({ kind: "statusCode", property });
return createResult({ kind: "statusCode" });
} else if (annotations.body) {
return createResult({ kind: "body", property });
return createResult({ kind: "body" });
} else if (annotations.bodyRoot) {
return createResult({ kind: "bodyRoot", property });
return createResult({ kind: "bodyRoot" });
} else if (annotations.multipartBody) {
return createResult({ kind: "multipartBody", property });
return createResult({ kind: "multipartBody" });
}
compilerAssert(false, `Unexpected http property type`, property);
compilerAssert(false, `Unexpected http property type`);
}

/**
Expand All @@ -161,53 +164,57 @@ export function resolvePayloadProperties(
}

const visited = new Set();
const queue = new Queue<[Model, ModelProperty | undefined]>([[type, undefined]]);

while (!queue.isEmpty()) {
const [model, rootOpt] = queue.dequeue();
function checkModel(model: Model, path: string[]) {
visited.add(model);

let foundBody = false;
let foundBodyProperty = false;
for (const property of walkPropertiesInherited(model)) {
const root = rootOpt ?? property;
const propPath = [...path, property.name];

if (!isVisible(program, property, visibility)) {
continue;
}

let httpProperty = diagnostics.pipe(getHttpProperty(program, property, options));
let httpProperty = diagnostics.pipe(getHttpProperty(program, property, propPath, options));
if (shouldTreatAsBodyProperty(httpProperty, visibility)) {
httpProperty = { kind: "bodyProperty", property };
httpProperty = { kind: "bodyProperty", property, path: propPath };
}
httpProperties.set(property, httpProperty);

if (
property !== root &&
(httpProperty.kind === "body" ||
httpProperty.kind === "bodyRoot" ||
httpProperty.kind === "multipartBody")
httpProperty.kind === "body" ||
httpProperty.kind === "bodyRoot" ||
httpProperty.kind === "multipartBody"
) {
const parent = httpProperties.get(root);
if (parent?.kind === "bodyProperty") {
httpProperties.delete(root);
}
}
if (httpProperty.kind === "body" || httpProperty.kind === "multipartBody") {
continue; // We ignore any properties under `@body` or `@multipartBody`
foundBody = true;
}

if (
property.type.kind === "Model" &&
!type.indexer &&
type.properties.size > 0 &&
!(httpProperty.kind === "body" || httpProperty.kind === "multipartBody") &&
isModelWithProperties(property.type) &&
!visited.has(property.type)
) {
queue.enqueue([property.type, root]);
if (checkModel(property.type, propPath)) {
foundBody = true;
continue;
}
}
if (httpProperty.kind === "bodyProperty") {
foundBodyProperty = true;
}
httpProperties.set(property, httpProperty);
}
return foundBody && !foundBodyProperty;
}

checkModel(type, []);

return diagnostics.wrap([...httpProperties.values()]);
}

function isModelWithProperties(type: Type): type is Model {
return type.kind === "Model" && !type.indexer && type.properties.size > 0;
}

function shouldTreatAsBodyProperty(property: HttpProperty, visibility: Visibility): boolean {
if (visibility & Visibility.Read) {
return property.kind === "query" || property.kind === "path";
Expand Down
1 change: 1 addition & 0 deletions packages/http/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export { HttpPartOptions } from "../generated-defs/TypeSpec.Http.Private.js";
export * from "./auth.js";
export * from "./content-types.js";
export * from "./decorators.js";
export type { HttpProperty } from "./http-property.js";
export * from "./metadata.js";
export * from "./operations.js";
export * from "./parameters.js";
Expand Down
1 change: 1 addition & 0 deletions packages/http/src/parameters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ function getOperationParametersForVerb(
const body = resolvedBody;

return diagnostics.wrap({
properties: metadata,
parameters,
verb,
body,
Expand Down
3 changes: 2 additions & 1 deletion packages/http/src/responses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,10 @@ function processResponseType(
response.responses.push({
body: resolvedBody,
headers,
properties: metadata,
});
} else {
response.responses.push({ headers });
response.responses.push({ headers, properties: metadata });
}
responses.set(statusCode, response);
}
Expand Down
8 changes: 7 additions & 1 deletion packages/http/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
Tuple,
Type,
} from "@typespec/compiler";
import { HeaderProperty } from "./http-property.js";
import { HeaderProperty, HttpProperty } from "./http-property.js";

/**
* @deprecated use `HttpOperation`. To remove in November 2022 release.
Expand Down Expand Up @@ -332,6 +332,9 @@ export type HttpOperationRequestBody = HttpOperationBody;
export type HttpOperationResponseBody = HttpOperationBody;

export interface HttpOperationParameters {
/** Http properties */
readonly properties: HttpProperty[];

parameters: HttpOperationParameter[];

body?: HttpOperationBody | HttpOperationMultipartBody;
Expand Down Expand Up @@ -441,6 +444,9 @@ export interface HttpOperationResponse {
}

export interface HttpOperationResponseContent {
/** Http properties for this response */
readonly properties: HttpProperty[];

headers?: Record<string, ModelProperty>;
body?: HttpOperationBody | HttpOperationMultipartBody;
}
Expand Down
24 changes: 24 additions & 0 deletions packages/http/test/parameters.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,30 @@ it("emit diagnostic when there is an unannotated parameter and a @body param", a
});
});

it("emit diagnostic when there is an unannotated parameter and a nested @body param", async () => {
const [_, diagnostics] = await compileOperations(`
@get op get(param1: string, nested: {@body param2: string}): void;
`);

expectDiagnostics(diagnostics, {
code: "@typespec/http/duplicate-body",
message:
"Operation has a @body and an unannotated parameter. There can only be one representing the body",
});
});

it("emit diagnostic when there is annotated param and @body nested together", async () => {
const [_, diagnostics] = await compileOperations(`
@get op get(nested: {param1: string, @body param2: string}): void;
`);

expectDiagnostics(diagnostics, {
code: "@typespec/http/duplicate-body",
message:
"Operation has a @body and an unannotated parameter. There can only be one representing the body",
});
});

it("emit diagnostic when there are multiple @body param", async () => {
const [_, diagnostics] = await compileOperations(`
@get op get(@query select: string, @body param1: string, @body param2: string): string;
Expand Down
Loading

0 comments on commit 7b5d263

Please sign in to comment.