Skip to content

Add URI resolution helpers for base URI resolution#489

Open
ayushshrivastv wants to merge 7 commits intomattpolzin:mainfrom
ayushshrivastv:BaseURIHelpers.swift
Open

Add URI resolution helpers for base URI resolution#489
ayushshrivastv wants to merge 7 commits intomattpolzin:mainfrom
ayushshrivastv:BaseURIHelpers.swift

Conversation

@ayushshrivastv
Copy link
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
Contributor Author

Implementation notes for reviewers:

  • Added shared URI-resolution helpers in both modules so callers can resolve URI references without reimplementing base-URI logic. JSONReference now exposes uriReference and resolvedURI(relativeTo:), and OpenAPI.Reference in OpenAPIKit forwards to the same logic.
  • Added document-level convenience helpers. In OpenAPIKit, OpenAPI.Document.baseURI(relativeTo:) establishes the document base URI by combining the retrieval URI with $self when present. In OpenAPIKit30, the retrieval URI is used directly since 3.0 documents do not support $self.
  • Added a small shared URL helper in OpenAPIKitCore so relative and fragment-only URI references are resolved consistently from one place.
  • Added schema base-URI support through JSONSchemaContext.baseURI(relativeTo:), and added id support on the OpenAPIKit schema context so schema identifiers can participate in base-URI composition through the public API.
  • Added focused regression coverage for:
    • JSONReferenceURIResolutionTests
    • DocumentURIResolutionTests
    • JSONSchemaURIResolutionTests

Files with the main implementation:

  • Sources/OpenAPIKitCore/Utility/URL+Resolution.swift
  • Sources/OpenAPIKit/JSONReference+URIResolution.swift
  • Sources/OpenAPIKit30/JSONReference+URIResolution.swift
  • Sources/OpenAPIKit/Document/Document+URIResolution.swift
  • Sources/OpenAPIKit30/Document/Document+URIResolution.swift
  • Sources/OpenAPIKit/Schema Object/JSONSchemaContext.swift
  • Sources/OpenAPIKit/Schema Object/JSONSchema.swift
  • Sources/OpenAPIKit30/Schema Object/JSONSchemaContext.swift

Local verification run on this branch:

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

@ayushshrivastv
Copy link
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
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
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
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.

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