chore: add e2e tests for security propagation during join#2827
Conversation
|
Performance Benchmark (Lower is Faster)
|
Coverage Report
File CoverageNo changed files found. |
|
@cursor review |
|
@cursor review |
|
@cursor review |
5bc9605 to
ecc35f1
Compare
tatomyr
left a comment
There was a problem hiding this comment.
Please also address the bugbot comments.
| } from '../../utils/miscellaneous.js'; | ||
| import type { CommandArgs } from '../../wrapper.js'; | ||
| import { COMPONENTS } from '../split/constants.js'; | ||
| // import { COMPONENTS } from '../split/constants.js'; |
| }, | ||
| }); | ||
| } | ||
| if (!security && openapi.hasOwnProperty('security')) { |
There was a problem hiding this comment.
Why the previous solution was not working? I see it refers to the root security already.
There was a problem hiding this comment.
Yeah, good point. Previous solution was working for each case, except when we have paths: {} and security on root level. Simplified the solution to cover this case, please, review one more time.
11a4935 to
09e577e
Compare
| 200: | ||
| description: OK | ||
| 400: | ||
| description: Bad request No newline at end of file |
There was a problem hiding this comment.
| description: Bad request | |
| description: Bad request | |
Let's add a newline on file endings.
There was a problem hiding this comment.
Added to each file
| info: | ||
| title: "spec1" | ||
| version: 1.0.0 | ||
| servers: |
There was a problem hiding this comment.
Let's add only essential fields and remove others.
| servers: | ||
| - url: https://api.example.com | ||
| paths: | ||
| /post: |
There was a problem hiding this comment.
It's bad practice to use verbs in paths (especially mixing post and get), so I'm against using it in our examples.
| description: Bad request | ||
| tags: | ||
| - bar_other | ||
| security: |
There was a problem hiding this comment.
If I'm getting it correctly, this path comes from an API description that doesn't have security defined on it; so how it got the security requirement from another description?
There was a problem hiding this comment.
Yes, you are right, updated tests
| authorizationUrl: https://example.com/oauth/authorize | ||
| tokenUrl: https://example.com/oauth/token | ||
| scopes: {} | ||
| oauth1: |
There was a problem hiding this comment.
These names are hard to follow. Please use something more meaningful.
b887c33 to
1ecab94
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ab620eb. Configure here.
c4ad9b7 to
4b8bca5
Compare
4b8bca5 to
cdac599
Compare
cdac599 to
5f66807
Compare

What/Why/How?
Added e2e tests for security propagation during join
Reference
#1409
Testing
Screenshots (optional)
Check yourself
Security
Note
Low Risk
Only new e2e fixtures and snapshots; no production join or security logic is modified in this diff.
Overview
Adds join e2e coverage for how root-level
securitybehaves when merging OpenAPI specs, tied to #1409.Two new parameterized cases are registered in
join.test.ts:root-security-in-both-specs(each input has its own rootsecurityand paths) androot-security-without-paths(one spec has rootsecuritybut emptypaths). Each case includesfoo.yaml/bar.yamlfixtures and asnapshot.txtgolden output from the CLIjoincommand.The snapshots document expected behavior: per-operation
securityis applied from each source’s root requirement (e.g.oauth2vsbearerAuth),components.securitySchemesare merged, and rootsecurityis not left on the merged document when operations carry the effective requirements.Reviewed by Cursor Bugbot for commit 525a38c. Bugbot is set up for automated code reviews on this repo. Configure here.