Skip to content

[MISSING_APIS_IN_DEFAULT_TAG] False positive if readme.md uses windows paths instead of posix paths #167

@mikeharder

Description

@mikeharder

https://github.com/Azure/azure-rest-api-specs-pr/blob/34c96e47be718063d4e2de4412cc4b1ee9f7d149/specification/monitor/resource-manager/Microsoft.Monitor/PipelineGroups/readme.md?plain=1

This readme incorrect used windows paths (backslash) instead of posix paths (forward-slash).

Expected:

input-file:
- preview/2024-10-01-preview/pipelineGroups.json

Actual:

input-file:
- preview\2024-10-01-preview\pipelineGroups.json

Strangely, most of Avocado worked correctly, but it caused false positives in rule MISSING_APIS_IN_DEFAULT_TAG.

Workaround is to use posix paths in readme.

Fix is either:

  1. Fail fast if readme contains windows paths
  2. Ensure paths are normalized before comparisons
    • Most Avocado rules seemed to work correctly, so maybe bug is specific to MISSING_APIS_IN_DEFAULT_TAG.

Root Cause

I believe the root cause is in dep @azure/openapi-markdown. This returns the input-file "strings", but doesn't attempt to resolve or normalize them:

https://github.com/Azure/openapi-markdown/blob/58d65b9ccc1effd3d206a171e6d893cf3b9074b0/src/readMeManipulator.ts#L167-L168

We might be able to fix/workaround in Avocado, by either normalize (or validating) paths after calling this API:

avocado/src/index.ts

Lines 209 to 216 in 0576dad

export const getSwaggerFileUnderDefaultTag = (m: md.MarkDownEx): string[] => {
const defaultTag = getDefaultTag(m.markDown)
if (!defaultTag) {
return []
}
const inputFiles = openApiMd.getInputFilesForTag(m.markDown, defaultTag)
return (inputFiles as any) || []
}

avocado/src/index.ts

Lines 392 to 402 in 0576dad

const inputFiles = getSwaggerFileUnderDefaultTag(m)
let defaultTagPathTable = new Map<string, { apiVersion: string; swaggerFile: string }>()
let stableCheck = false
let previewCheck = false
for (const inputFile of inputFiles) {
stableCheck = stableCheck || inputFile.includes('stable')
previewCheck = previewCheck || inputFile.includes('preview')
const inputFilePath = path.resolve(readmeDir, inputFile)
const pathTable = getPathTableFromSwaggerFile(inputFilePath)
defaultTagPathTable = mergePathTable(defaultTagPathTable, pathTable)
}

Another question, if a file doesn't exist, would it be better for Avocado to throw immediately, rather than silently using an empty map?

avocado/src/index.ts

Lines 491 to 494 in 0576dad

export const getPathTableFromSwaggerFile = (swaggerFile: string): PathTable => {
if (!fs.existsSync(swaggerFile)) {
return new Map<string, { apiVersion: string; swaggerFile: string }>()
}

Our new code in spec-model attempts to normalize and resolve, so it should always return platform-specific absolute paths. Though even this code might be sub-optimal or have bugs.

https://github.com/Azure/azure-rest-api-specs/blob/63ea5930957bc33f38a9f1071c7c41a07eea80d4/.github/shared/src/readme.js#L156-L158

https://github.com/Azure/azure-rest-api-specs/blob/63ea5930957bc33f38a9f1071c7c41a07eea80d4/.github/shared/src/readme.js#L78-L85

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

Status

📋 Backlog

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions