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
[Security Solution] Adjust OpenAPI specs bundler for doc requirements #181944
[Security Solution] Adjust OpenAPI specs bundler for doc requirements #181944
Conversation
0dc5fa6
to
473f05c
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
d2584d2
to
6aedaf1
Compare
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.
Thank you, @maximpn, for this awesome improvement to the bundler, it's really huge 🎉 I tested the PR locally, and the bundled Security Solution APIs look correct. I was able to easily export all our APIs at once to Postman, and this is 🔥🔥🔥 It's a big step forward for our tooling.
I left several comments regarding tests that produce invalid OpenAPI, but that doesn't seem to affect our API bundle, though. Maybe it's just the tests themselves that need to be fixed. Also, I've made a couple of comments regarding the readability and maintainability of the bundler code, so please take a look, and I'll be happy to discuss this in more detail if needed.
...i-bundler/src/bundler/document_processors/reduce_all_of_items/flatten_folded_all_of_items.ts
Outdated
Show resolved
Hide resolved
*/ | ||
export function createFlattenFoldedAllOfItemsProcessor(): DocumentNodeProcessor { | ||
return { | ||
leave(node) { |
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.
Why is this method called leave
? It is somewhat confusing because it's used to modify a node. Let's rename it to something more straightforward, like modify
.
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.
There are two phased at which OpenAPI document is traversed.
- At the first phase diving from root to leaves happens (like event's capture phase in DOM). It means we
enter
a subtree. Processing isn't happening here so it's convenient to analyze not yet processed nodes. For example understand which nodes should be inlined. - At the second phase bubbling from leaves to root happens (like event's bubble phase in DOM). It means we
leave
s subtree. It's possible to modify a node during this phase while it's not necessary.
This naming highlights the flexibility. enter
and leave
methods could be used to collect metrics or analyze the document. Current implementation doesn't have such functionality but it could be added without any effort.
I extended a comment for DocumentNodeProcessor
in packages/kbn-openapi-bundler/src/bundler/types.ts
. While we don't expose DocumentNodeProcessor API I think package README shouldn't be updated.
...i-bundler/src/bundler/document_processors/reduce_all_of_items/flatten_folded_all_of_items.ts
Outdated
Show resolved
Hide resolved
...er/src/bundler/document_processors/reduce_all_of_items/merge_non_conflicting_all_of_items.ts
Outdated
Show resolved
Hide resolved
...er/src/bundler/document_processors/reduce_all_of_items/merge_non_conflicting_all_of_items.ts
Outdated
Show resolved
Hide resolved
...uce_all_of/merge_all_of_items_with_inlined_refs_in_different_document_branches/expected.yaml
Outdated
Show resolved
Hide resolved
..._all_of/merge_all_of_items_with_inlined_refs_in_different_document_branches/spec.schema.yaml
Outdated
Show resolved
Hide resolved
// Fails because `writeYamlDocument()` has `noRefs: true` setting | ||
// it('bundles recursive spec', async () => { | ||
// const folder = join('circular', 'recursive_spec'); | ||
|
||
// await bundleFolder(folder); | ||
// await expectBundleToMatchFile(DEFAULT_BUNDLED_FILE_PATH, join(folder, 'expected.yaml')); | ||
// }); |
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.
Let's remove the commented code and the recursive_spec
files
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.
I managed to uncomment the test by allowing to write documents with circular references by modifying writeYamlDocument
.
}); | ||
|
||
describe('remove props', () => { | ||
it('removes "x-codegen-enabled" property', async () => { |
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.
Should we also check that other x-
properties are getting removed?
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.
I've added extra tests.
); | ||
|
||
await bundleFolder(folder); | ||
await expectBundleToMatchFile(DEFAULT_BUNDLED_FILE_PATH, join(folder, 'expected.yaml')); |
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.
Getting to the last test in this file, I started understanding that this snapshot-like testing requires quite a lot of concentration and effort to read through and understand what is being covered. For each test, you need to read the description here, then navigate to the test folder, locate the source files and the expected YAML, then open them side by side, and compare their content to see if the input matches the expected output. This process is pretty error-prone, and as we've seen above, snapshots just captured invalid output, leaving it up to humans to verify its correctness.
I wonder what options we have to improve this experience. Maybe we could inline some of the test cases that are simple enough so the input and output are easily visible and reduce the boilerplate in the test files? What else could we do?
6aedaf1
to
b0c93da
Compare
49e35c6
to
810d5bc
Compare
After syncing with @xcrzx via Zoom we discussed improvements to maintainability. The following has been decided and addressed
|
9c70c5e
to
ee0c313
Compare
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.
Entity Analytics changes LGTM!
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @maximpn |
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.
Reviewed and tested locally, everything works as expected 👍
And thank you for updating the tests, @maximpn, they are much easier to read and follow the logic now 🙌
Addresses: https://github.com/elastic/security-team/issues/7981
Relates to: #171526
Summary
This PR adjusts OpenAPI Bundler (
kbn-openapi-bundler
package) based on Docs Engineering team requirements. Main requirements include producing one valid OpenAPI spec bundle file. After adjustments OpenAPI Bundler one valid OpenAPI spec bundle file per API version e.g.2023-10-31-my-oas.bundled.schema.yaml
.Details
This PR further improves #171526 to satisfy Docs Engineering team requirements and includes the following adjustments and improvements
outputFilePath
independent fromrootDir
outputFilePath
can be an absolute or relative to node.js working directory path. Additionally it can contain{version}
placeholder to adjust where API version is placed in the result bundle file name. API prepends the result bundle file name if{version}
is omitted.allOf
itemsInlined schemas lead to
allOf
folder into each other as well as having multiple object schemas which could be merged. Docs Engineering team stated potential problems related to foldedallOf
items.3.0.3
and3.1.0
summary
,description
andproperties
schemas
)Besides the changes above PR contains minor bug fixing and improvements.
Note: It intentionally doesn't include bundling configuration changes like integration it into PR's build pipeline and focuses only on functional changes. Bundling configuration will be addressed separately to avoid spreading reviewers focus.