Emit diagnostic for any referenced operation with a container route prefix #2337
Conversation
daviwil
requested review from
allenjzhang,
bterlson,
markcowl and
timotheeguerin
as code owners
|
Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status): Website: https://cadlwebsite.z1.web.core.windows.net/prs/2337/ |
packages/http/src/validate.ts
Outdated
| import { getRoutePath, isSharedRoute } from "./route.js"; | ||
| import { OperationContainer } from "./types.js"; | ||
|
|
||
| function checkOperationReferenceContainerRoutes(program: Program) { |
There was a problem hiding this comment.
I believe this should be a linter rule warning. $onValidate is typically reserved for errors.
There was a problem hiding this comment.
That's correct. With the new linter rule system only errors should be in onValidate. This does look like it should be a linter rule warning.
There was a problem hiding this comment.
Alright, I'll work on that. There wasn't any existing linter rule infra in this package yet so I took the easy route.
There was a problem hiding this comment.
OK, I've looked at @catalinaperalta's changes to migrate the rules in typespec-azure and it makes sense, but I wonder if we start exposing linter rules from @typespec/http if we have to add an extends for it in typespec-azure-core. What do you think @catalinaperalta?
EDIT: By that same logic, we might need to pre-emptively add the ruleset of @typespec/rest too because there's a direct relationship through those libraries to Azure.Core.
There was a problem hiding this comment.
Updated the design to use the new linting framework instead. @timotheeguerin, how does it look? Also, can you answer the question I asked above?
There was a problem hiding this comment.
@daviwil Yes we'd need to extend this in typespec-azure-core to keep checking for the warnings coming from @typespec/http.
| @@ -0,0 +1,28 @@ | |||
| // FIXME - This is a workaround for the circular dependency issue when loading | |||
There was a problem hiding this comment.
:( showing up everywhere now
There was a problem hiding this comment.
Yeah, it took me a while to figure out what was going on there. I happened to catch the workaround from the ARM library
| import { OperationContainer } from "../types.js"; | ||
|
|
||
| export const operationReferenceContainerRouteRule = createRule({ | ||
| name: "operation-reference-container-route", |
There was a problem hiding this comment.
| name: "operation-reference-container-route", | |
| name: "op-reference-container-route", |
nit: would be a little nicer to use the short name for those?
There was a problem hiding this comment.
Yeah, it's a bit long
timotheeguerin
left a comment
timotheeguerin
left a comment
There was a problem hiding this comment.
Looks great I think we probably want this to be added to the azure core and arm rulesets too.
packages/http/src/lib.ts
Outdated
| ruleSets: { | ||
| all: { | ||
| enable: { | ||
| [`@typespec/http/${operationReferenceContainerRouteRule.name}`]: true, |
There was a problem hiding this comment.
I feel like we need to add this rule to the azure-core ruleset in a separate PR
There was a problem hiding this comment.
Sent PR Azure/typespec-azure#3546 to add this
5 participants
This change fixes Azure/typespec-azure#3333 by adding a validation step in the
@typespec/httplibrary which checks whether any referenced operation lives inside of an interface or namespace with a@routeprefix defined.This check is intended to help a spec author avoid the case where an operation loses the prefix of its referenced operation because it is used outside of the original container. The issue was originally raised in issue #1466.