Skip to content

docs(converter-openapi): align README extension table with code#447

Open
kayx23 wants to merge 2 commits into
mainfrom
fix/converter-openapi-readme-drift
Open

docs(converter-openapi): align README extension table with code#447
kayx23 wants to merge 2 commits into
mainfrom
fix/converter-openapi-readme-drift

Conversation

@kayx23
Copy link
Copy Markdown
Contributor

@kayx23 kayx23 commented May 25, 2026

Fixes #446.

Three long-standing inaccuracies in libs/converter-openapi/README.md's supported-extensions table, identified by cross-checking each row against src/schema.ts, src/parser.ts, src/index.ts, src/extension.ts, and the fixtures under test/assets/.

1. x-adc-labels was documented at Path level but is not honored there

The Zod schema only defines x-adc-labels on the root document and on each operation object — there is no entry for it in the path-item schema:

https://github.com/api7/adc/blob/main/libs/converter-openapi/src/schema.ts#L58-L75

The path-item is a z.looseObject, so a path-level x-adc-labels: {...} passes validation, but src/index.ts never reads operations[ExtKey.LABELS] — only oas[ExtKey.LABELS] (root) and operation[ExtKey.LABELS] (operation) are mapped through:

https://github.com/api7/adc/blob/main/libs/converter-openapi/src/index.ts#L44
https://github.com/api7/adc/blob/main/libs/converter-openapi/src/index.ts#L95

Result: path-level labels are silently dropped at conversion. Dropping the Path row from the table.

2. x-adc-plugins at Path/Operation was documented as causing a service split, but does not

The actual splitting condition in parseSeprateService only fires on x-adc-service-defaults, x-adc-upstream-defaults, or a sub-level servers: entry:

https://github.com/api7/adc/blob/main/libs/converter-openapi/src/parser.ts#L60-L65

Plugins are merged into route.plugins directly in src/index.ts (lines 82-85, 102) without invoking parseSeprateService. The test fixtures extension-5.yaml and extension-6.yaml both produce a single service in the output despite carrying plugins at root, path, and operation levels. Rewriting the description to match.

3. x-adc-route-defaults was documented as causing a service split, but does not

Same root cause as #2 — the field is not on the splitting checklist in parseSeprateService. It is merged into each route via structuredClone spreads in src/index.ts:

https://github.com/api7/adc/blob/main/libs/converter-openapi/src/index.ts#L103-L105

The order is root → path → operation, with operation winning. Verified against test/assets/extension-7.yaml: the GET-only service split in that fixture is caused by an operation-level servers: block, not by x-adc-route-defaults; the PUT operation keeps its merged route-defaults on the main service without splitting. Updating the description.

Note for reviewers

I chose to fix the README rather than the code because the current behavior (no splitting on plugins or route-defaults; no path-level labels) looks intentional and the test suite encodes it. If any of these is actually a wanted feature, this PR can become a starting point for that conversation — happy to take the other route.

Context

Surfaced while writing downstream documentation in api7/docs#1638. Posting the README fix here so the upstream source of truth stops misleading users.

Summary by CodeRabbit

  • Documentation
    • Clarified that x-adc-labels is supported only at Root and Operation levels (Path level removed).
    • Revised x-adc-plugins wording to remove the previous claim that Path/Operation-level plugin objects cause service splitting.
    • Updated x-adc-route-defaults to state it does not by itself cause service splitting.

Review Change Stack

Two long-standing inaccuracies in the supported-extensions table:

1. x-adc-labels was documented at Root / Path / Operation levels but
   the Zod schema in src/schema.ts only defines it at the root document
   and on each operation object. There is no entry for x-adc-labels on
   the path-item schema, and src/index.ts only reads oas[ExtKey.LABELS]
   (root, line 44) and operation[ExtKey.LABELS] (operation, line 95).
   Path-level x-adc-labels passes loose-object validation but is then
   silently dropped at conversion. Drop the Path row from the table.

2. x-adc-plugins was documented as causing the service to be split when
   present at Path or Operation level. The actual splitting condition
   in src/parser.ts parseSeprateService (lines 60-65) only fires on
   x-adc-service-defaults, x-adc-upstream-defaults, or a sub-level
   servers: entry. Plugins are merged into route.plugins directly in
   src/index.ts (lines 82-85, 102) without invoking parseSeprateService.
   Test fixtures extension-5.yaml and extension-6.yaml both produce a
   single service in the output despite carrying plugins at every
   level. Update the description to match.

Fixes #446
@kayx23 kayx23 requested a review from bzp2010 as a code owner May 25, 2026 08:52
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 25, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ test
❌ kayx23


test seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


test seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ec09d54a-43b7-4876-bd48-45029305db27

📥 Commits

Reviewing files that changed from the base of the PR and between 494d76d and d9d806e.

📒 Files selected for processing (1)
  • libs/converter-openapi/README.md

📝 Walkthrough

Walkthrough

This PR edits the OpenAPI converter README: it removes Path Level from x-adc-labels, revises x-adc-plugins to drop the service-splitting claim for Path/Operation levels, and updates x-adc-route-defaults to state it does not by itself cause service splitting.

Changes

Extension Fields Documentation

Layer / File(s) Summary
Extension fields table updates
libs/converter-openapi/README.md
Updated table rows: x-adc-labels now lists only Root and Operation levels; x-adc-plugins no longer claims Path/Operation-level plugins cause service splitting; x-adc-route-defaults clarified to not by itself cause splitting.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: updating the README to align with the actual code implementation.
Linked Issues check ✅ Passed The PR directly addresses all objectives from issue #446: removes Path level from x-adc-labels support and corrects x-adc-plugins splitting claim.
Out of Scope Changes check ✅ Passed All changes are within scope: the PR updates only the README table to correct documented inaccuracies, with no extraneous modifications.
E2e Test Quality Review ✅ Passed This PR is documentation-only (README.md update) addressing issue #446 discrepancies. E2E test quality check is not applicable to documentation changes; the check is designed for code/feature changes.
Security Check ✅ Passed PR contains only README documentation updates with no code changes; security check is not applicable to documentation-only changes with no executable code, configuration, or secrets.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/converter-openapi-readme-drift

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…the service

Third inaccuracy in the same table: x-adc-route-defaults was described as 'This field on sub-levels will cause the service to be split.' The splitting condition in parseSeprateService (src/parser.ts lines 60-65) only checks SERVICE_DEFAULTS, UPSTREAM_DEFAULTS, and sub-level servers: — not ROUTE_DEFAULTS. The field is merged into each route via structuredClone spreads in src/index.ts lines 103-105 (root -> path -> operation order, operation wins).

Verified against test/assets/extension-7.yaml: the GET service split in that fixture is caused by an operation-level servers: block, not by x-adc-route-defaults; the PUT operation keeps its merged route-defaults on the main service without splitting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

libs/converter-openapi/README.md disagrees with the converter on x-adc-labels path level and x-adc-plugins splitting

2 participants