-
Notifications
You must be signed in to change notification settings - Fork 374
Detect mock api uri / spec route mismatches in http-specs validation #10978
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
a5fa578
ad58416
4d84c27
8c115cb
f00c745
3f9b3c6
d0cfcf0
b152f41
b54a8df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: fix | ||
| packages: | ||
| - "@typespec/http-specs" | ||
| --- | ||
|
|
||
| Fix the swapped mock api uris for the `Routes_fixed` and `Routes_InInterface` scenarios so they match the routes defined in the spec. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: fix | ||
| packages: | ||
| - "@typespec/spector" | ||
| --- | ||
|
|
||
| `validate-mock-apis` now verifies that every route defined in a scenario's `main.tsp` is served by at least one of the scenario's mock API `uri`s, so a mismatch between the spec route and the mock api uri (which would make a generated client get a 404 from the mock server) is detected by CI. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,185 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| getServerPathPrefixSegmentCount, | ||
| isMockApiUriConsistentWithRoute, | ||
| normalizeMockApiUri, | ||
| } from "./route-utils.js"; | ||
|
|
||
| describe("normalizeMockApiUri", () => { | ||
| it("drops the query string", () => { | ||
| expect(normalizeMockApiUri("/foo/bar?baz=1")).toBe("/foo/bar"); | ||
| }); | ||
|
|
||
| it("removes backslash escapes", () => { | ||
| expect(normalizeMockApiUri("/versioning/removed/api-version\\:v1/v3")).toBe( | ||
| "/versioning/removed/api-version:v1/v3", | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("getServerPathPrefixSegmentCount", () => { | ||
| it("returns 0 for an undefined server", () => { | ||
| expect(getServerPathPrefixSegmentCount(undefined)).toBe(0); | ||
| }); | ||
|
|
||
| it("returns 0 for the default localhost server", () => { | ||
| expect(getServerPathPrefixSegmentCount("http://localhost:3000")).toBe(0); | ||
| }); | ||
|
|
||
| it("returns 0 for a bare host server (e.g. ARM)", () => { | ||
| expect(getServerPathPrefixSegmentCount("https://management.azure.com")).toBe(0); | ||
| }); | ||
|
|
||
| it("counts the path segments after an {endpoint} template", () => { | ||
| expect( | ||
| getServerPathPrefixSegmentCount( | ||
| "{endpoint}/resiliency/service-driven/client:v2/service:{serviceDeploymentVersion}/api-version:{apiVersion}", | ||
| ), | ||
| ).toBe(5); | ||
| }); | ||
|
|
||
| it("counts the path segments after localhost:3000", () => { | ||
| expect(getServerPathPrefixSegmentCount("http://localhost:3000/my/prefix")).toBe(2); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isMockApiUriConsistentWithRoute", () => { | ||
| it("matches identical routes", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute("/parameters/basic/simple", "/parameters/basic/simple"), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
|
msyyc marked this conversation as resolved.
|
||
| it("detects a mismatch in a literal segment", () => { | ||
| // A literal route segment must match exactly: `dollarSign` (camelCase) must not be considered | ||
| // consistent with a `dollar-sign` (kebab-case) uri. | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/parameters/query/special-char/dollarSign", | ||
| "/parameters/query/special-char/dollar-sign", | ||
| ), | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| it("detects swapped routes", () => { | ||
| expect(isMockApiUriConsistentWithRoute("/routes/fixed", "/routes/in-interface/fixed")).toBe( | ||
| false, | ||
| ); | ||
| }); | ||
|
|
||
| it("treats whole-segment template expressions as wildcards", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/routes/path/template-only/{param}", | ||
| "/routes/path/template-only/a", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("matches template expressions embedded in a segment", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/routes/path/simple/standard/primitive{param}", | ||
| "/routes/path/simple/standard/primitivea", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("matches path expansion template expressions that expand to extra segments", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/routes/path/path/standard/primitive{param}", | ||
| "/routes/path/path/standard/primitive/a", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("detects a uri that is missing a trailing literal segment present in the route", () => { | ||
| expect(isMockApiUriConsistentWithRoute("/parameters/basic/simple", "/parameters/basic")).toBe( | ||
| false, | ||
| ); | ||
| }); | ||
|
|
||
| it("ignores trailing slashes", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/azure/special-headers/x-ms-client-request-id/", | ||
| "/azure/special-headers/x-ms-client-request-id", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("ignores the query string of both the route and the uri", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if the query string are inconsitent? I think because typespec routes are uri template, the query is be part of uri and the mockapi, should we check for that?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point — the query is part of the uri template, and right now the check strips it, so a constant param like fixed=true would slip through. I scoped this PR to path-level mismatches (where our silent 404s come from); query validation is trickier (order-independent, explode, reserved expansions) and lower value since only constant params can diverge. Above all, I'd prefer to track it as a follow-up rather than expand scope here if you want.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fine by me |
||
| "/routes/query/query-continuation/standard/primitive?fixed=true", | ||
| "/routes/query/query-continuation/standard/primitive?fixed=true¶m=a", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("matches routes containing escaped characters in the uri", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/versioning/removed/api-version:{version}/v3", | ||
| "/versioning/removed/api-version\\:v1/v3", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| describe("with a server path prefix", () => { | ||
| // Resiliency service-driven: the api-version/client/service version segments come from the | ||
| // `@server` url and may legitimately differ from the mock uri (e.g. client:v1 vs client:v2). | ||
| const serverPrefix = getServerPathPrefixSegmentCount( | ||
| "{endpoint}/resiliency/service-driven/client:v2/service:{serviceDeploymentVersion}/api-version:{apiVersion}", | ||
| ); | ||
|
|
||
| it("skips the server prefix segments before comparing", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/add-optional-param/from-none", | ||
| "/resiliency/service-driven/client\\:v1/service\\:v1/api-version\\:v1/add-optional-param/from-none", | ||
| serverPrefix, | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("still detects a mismatch in the operation route after the server prefix", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/add-optional-param/from-none", | ||
| "/resiliency/service-driven/client\\:v2/service\\:v2/api-version\\:v2/add-operation", | ||
| serverPrefix, | ||
| ), | ||
| ).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("ARM routes", () => { | ||
| it("matches a fully resolved ARM tracked resource route", () => { | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Azure.ResourceManager.Resources/topLevelTrackedResources/{topLevelTrackedResourceName}", | ||
| "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Azure.ResourceManager.Resources/topLevelTrackedResources/top", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("matches an extension resource route whose {resourceUri} spans several scope segments", () => { | ||
| const route = | ||
| "/{resourceUri}/providers/Azure.ResourceManager.Resources/extensionsResources/{extensionsResourceName}"; | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| route, | ||
| "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Azure.ResourceManager.Resources/extensionsResources/extension", | ||
| ), | ||
| ).toBe(true); | ||
| expect( | ||
| isMockApiUriConsistentWithRoute( | ||
| route, | ||
| "/providers/Azure.ResourceManager.Resources/extensionsResources/extension", | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did this just reorder them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the change is correct. It swaps two URIs that were assigned to each other's scenarios (Routes_fixed↔Routes_InInterface), confirmed against the Expected path docs in main.tsp. Not a cosmetic reorder.