Skip to content
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

Added a Validation that checks all operationIds in Link objects are valid #404

Merged

Conversation

ayushshrivastv
Copy link
Contributor

Issue #236

This PR directly addresses the issue mentioned in the GitHub project (issue #236) for OpenAPIKit.

I have added a validation that ensures operationIds in Link objects refer to actual Operations that exist in the document.

Changes

  1. I've created a new optional validation called linkOperationsExist that verifies Link objects with operationIds are pointing to valid Operations in the document.

  2. Extracting the operationId from the Link object (ignoring Links that use operationRef instead)

  • Collecting all operationIds from all Operations in the document (using different approaches in OpenAPIKit and OpenAPIKit30)
  • Checking if the Link's operationId is contained in the list of valid operationIds
  1. When a Link refers to a non-existent operationId, the validation produces a clear error message indicating that the Link's operationId has no corresponding Operation in the document.

This validation is optional and not included in the default validator.

let validator = Validator.default.validating(.linkOperationsExist)
try document.validate(using: validator)

The PR completes the TODO item in the project while maintaining consistency with the existing validation framework, ensuring that OpenAPI documents can be properly validated for correctness in their Link references.

@mattpolzin
Copy link
Owner

Thanks! Superficially this looks good. I’ll take a closer look soon.

@mattpolzin
Copy link
Owner

Looks like I'll have to take a look at the pet store spec failures. Don't see that being related to this PR unless I missed a modification to that test file on read through.

@ayushshrivastv
Copy link
Contributor Author

If there’s anything I missed or need to change, please let me know. I’ll double check and make sure everything’s in order. I’ve already tested it locally, and the test cases are passing!

@mattpolzin mattpolzin self-assigned this Apr 6, 2025
@mattpolzin mattpolzin self-requested a review April 6, 2025 14:14
Copy link
Owner

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding some missing tests while you were in there! I left one question but nothing else looks amiss.

Comment on lines 417 to 420
let operationIds = context.document.paths.values
.compactMap { context.document.components[$0] }
.flatMap { $0.endpoints }
.compactMap { $0.operation.operationId }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use the allOperationIds helper here like you did for the OpenAPIKit module's validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right, we can use allOperationIds here as well. I overlooked that in the module validation—thanks for pointing it out!

@mattpolzin
Copy link
Owner

I've fixed the tests that were broken for no reason connected to this PR and merged that in.

@mattpolzin
Copy link
Owner

The one remaining test failure is related to the security requirements validation test you added. As best I can tell (and I don't have memory one way or the other) the validation you are testing does not exist. Did you intend to add one? The validity of security requirements is asserted while decoding documents, but there is not a Validation per se for it, it is built into the decoding process currently.

@ayushshrivastv
Copy link
Contributor Author

The one remaining test failure is related to the security requirements validation test you added. As best I can tell (and I don't have memory one way or the other) the validation you are testing does not exist. Did you intend to add one? The validity of security requirements is asserted while decoding documents, but there is not a Validation per se for it, it is built into the decoding process currently.

I took another look, and you’re right—there isn’t a separate validation step for security requirements; it’s handled implicitly during decoding. I had originally assumed there was a dedicated validation function for it, but that’s not the case.

I’ll go ahead and adjust the test (or remove it if it no longer makes sense in this context) and update the implementation accordingly.

ThankYou!

@ayushshrivastv
Copy link
Contributor Author

I noticed that there are three test failures in the test suite. It seems like the test failures are linked to the test_avoid_stack_overflow in the URLTemplateTests suite. When I check the test output, I can see that the test is failing with a stack overflow error. This is happening because the URL template parser is using a recursive approach to parse the URL, which is causing the stack to overflow when dealing with really long URLs.

The test is failing in three different test suites:

OpenAPIKit30Tests
OpenAPIKitTests
OpenAPIKitCoreTests

I’ve been trying my best to find the problem with my pull request, but I can’t seem to spot it. I think the issue might be with these two tests: func test_serverVariableDefaultExistsInEnum_fails() and func test_serverVariableDefaultExistsInEnum_succeeds(). If I’m overlooking something here, could you please point me to the line of code that’s causing the test failures? I’d really appreciate it!

@mattpolzin
Copy link
Owner

Where are you seeing 3 failures? I only see 1 failure reported in the CI output.

@ayushshrivastv
Copy link
Contributor Author

As I see, all checks have now passed! 😊

Thank you, @mattpolzin, for taking your time and assisting me!

@mattpolzin mattpolzin merged commit da7d3a5 into mattpolzin:main Apr 9, 2025
25 checks passed
@mattpolzin
Copy link
Owner

Thanks!

@ayushshrivastv
Copy link
Contributor Author

Hello @mattpolzin,

I'm currently working on the OpenAPI Integration with DocC project, where I’m building a tool to automatically generate DocC documentation from OpenAPI files. It uses OpenAPIKit to parse API specifications and SymbolGraph to map them into Swift compatible types ensuring consistency between the API definitions and the resulting documentation.

By eliminating the need for manual documentation updates, this project aims to save time, reduce errors, and streamline the workflow for Swift developers working with REST APIs.

I’d love to get your feedback on the initial approach. As you have better experience working with the OpenAPIKit library, your input would be incredibly valuable in guiding the next stages of this project.

You can find the ongoing discussion here: Swift Forums

Thank you!

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.

2 participants