Skip to content

Add URI resolution helpers for base URI resolution#489

Merged
mattpolzin merged 18 commits into
mattpolzin:mainfrom
ayushshrivastv:BaseURIHelpers.swift
May 14, 2026
Merged

Add URI resolution helpers for base URI resolution#489
mattpolzin merged 18 commits into
mattpolzin:mainfrom
ayushshrivastv:BaseURIHelpers.swift

Conversation

@ayushshrivastv
Copy link
Copy Markdown
Contributor

Summary

  • add reusable URI resolution helpers for raw URLs, JSON references, and documents in both OpenAPIKit and OpenAPIKit30
  • add schema base URI helpers and identifier support on OpenAPIKit schema contexts so callers can compose parent base URIs with schema identifiers
  • cover the new helpers with focused document, reference, and schema URI resolution tests

Closes #439.

Testing

  • swift test --filter JSONReferenceURIResolutionTests
  • swift test --filter DocumentURIResolutionTests
  • swift test --filter JSONSchemaURIResolutionTests
  • git diff --check

@ayushshrivastv
Copy link
Copy Markdown
Contributor Author

Follow-up for the failing linux (swift:6.1-jammy) job: the failure was not in the new URI-resolution code. It was the same older Swift async runtime crash in OpenAPI.PathItem.externallyDereferenced / OpenAPIKit30/Path Item/DereferencedPathItem.swift during ExternalDereferencingDocumentTests.test_example. I cherry-picked the proven compatibility fix (bd82590) onto this branch, pushed it, and re-ran the relevant coverage locally.

Additional local verification after the follow-up commit:

  • swift test --filter ExternalDereferencingDocumentTests/test_example -Xswiftc -strict-concurrency=complete
  • swift test --filter JSONReferenceURIResolutionTests
  • swift test --filter DocumentURIResolutionTests
  • swift test --filter JSONSchemaURIResolutionTests
  • git diff --check

Copy link
Copy Markdown
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.

Were these changes generated by an LLM? I ask because there are places where decoding of the same property is being done twice and in one of the two locations, the strategy being used does not fit the patterns established in this codebase.

If this PR was hand written, I am happy to walk through what I mean. However, if this PR was generated code that has not been read and understood by a human, I would request that you spend the time to work through this code on your own and attempt to spot the problem and gain further understanding of the codebase.

@ayushshrivastv
Copy link
Copy Markdown
Contributor Author

I put this change together myself while tracing through the existing code and validating it locally. Also I have used Codex at times to help work through failing test cases, but I still verify the changes myself before committing them. Your comment makes it clear that I’ve missed an inconsistency in how this part should be handled, so I’m going to revisit it carefully and align it with the patterns already used in the codebase before I push an update.

For context, I’ve contributed to this project before and have had a couple of PRs merged, including #405 and #404. I’ve also been keeping notes here: OpenAPI Integration with DocC.

@ayushshrivastv
Copy link
Copy Markdown
Contributor Author

I went back through the PR again and tightened the remaining pieces to stay aligned with the existing patterns in the codebase. In particular, $id now goes only through the existing CoreContext decoding path, and I cleaned up the schema accessor/initializer usage so it follows the same style already used in main rather than introducing a separate approach. I also reverted the async workaround changes, so this PR no longer changes the existing concurrency behavior in DereferencedPathItem.

Copy link
Copy Markdown
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.

I apologize I have not had much time for reviewing PRs in this repo lately. I gave this PR a look tonight with emphasis on understanding if the code adheres to the specification or not. I will have to take more time to look over the implementation itself later, but I wanted to suggest that you add some more test coverage for Document baseURI usage because there are several important combinations of retrieval URL and $self that Appendix F of the spec goes through as examples and it would be good to have test coverage to know the implementation at least treats all of those example situations correctly.

@ayushshrivastv
Copy link
Copy Markdown
Contributor Author

Thanks @mattpolzin for the overview! I’ll expand test coverage to ensure those scenarios are properly handled. There’s no urgency on this, so feel free to review at your convenience.

Copy link
Copy Markdown
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.

Sorry again for the delayed review. This PR looks like it's in good shape. I've got a couple of small comments or requests.

Comment thread Sources/OpenAPIKit/JSONReference+URIResolution.swift Outdated
Comment thread Sources/OpenAPIKitCore/Utility/URL+Resolution.swift Outdated
Comment thread Sources/OpenAPIKitCore/Utility/URL+Resolution.swift Outdated
@ayushshrivastv
Copy link
Copy Markdown
Contributor Author

@mattpolzin, I’ve addressed your feedbacks on this PR in this commit: 65941af

Thanks!

@mattpolzin
Copy link
Copy Markdown
Owner

Hey, thanks for the continued work on this. I wasn't quite thinking that I wanted to go for optional URL returns when I asked if there was any way we could avoid forcing the string to parse as a URL. Everything else is looking good I think.

What I was hoping for was a way to build a URL from component parts avoiding the need to force an optional because we would know the URL was going to be correct by construction. I went looking at the docs and was surprised (and sad) to see that this isn't actually possible. The closest it seems you can do is construct a URLComponents and then get a URL from that but you still end up with an optional URL even then... disappointing.

Can I request that we go back to non-optional URL returns but instead of forcing a string-parsed URL we create a URLComponents value by adding the fragment onto it and then use url! on that -- we are still forcing, but we are leaving a little bit up to chance than if we go via a plain string value.

@ayushshrivastv
Copy link
Copy Markdown
Contributor Author

Agreed, optional returns were a broader API change than needed here. I changed this back to non-optional URL returns and now construct the internal fragment URL with URLComponents, forcing only after the fragment has been set by construction.

@mattpolzin
Copy link
Copy Markdown
Owner

This looks good. Thanks for working with me on the URL construction while I figured out what was supported by the URL type itself.

@mattpolzin
Copy link
Copy Markdown
Owner

I just had one final thought which was that I would like to avoid adding a generic helper to the URL type given that helper is only used internally by OpenAPIKit and this library is not here to offer "url resolution relative to base urls" to downstream users of the URL type.

I just pushed a change that just moves that function out of an extension to keep the URL type cleaner.

@mattpolzin mattpolzin merged commit adc507f into mattpolzin:main May 14, 2026
12 checks passed
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.

build in helpers to resolve URIs against base URIs

2 participants