fix: correct Requesty model fetching URL construction #7379
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fix URL construction in getRequestyModels to properly append /models path - Ensure base URL with /v1 correctly resolves to /v1/models instead of /models - Add comprehensive tests for URL construction scenarios Fixes #7377
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| // Ensure the base URL ends with a slash so "models" is appended correctly | ||
| // Without this, new URL("models", "https://custom.requesty.ai/v1") would incorrectly | ||
| // resolve to "https://custom.requesty.ai/models" instead of "https://custom.requesty.ai/v1/models" | ||
| const baseWithSlash = resolvedBaseUrl.endsWith("/") ? resolvedBaseUrl : `${resolvedBaseUrl}/` |
Contributor
Author
There was a problem hiding this comment.
The fix looks correct and the comment is helpful! Though I'm wondering - what happens if someone provides a base URL with query parameters like https://custom.requesty.ai/v1?key=value? The URL constructor should handle it, but might be worth adding a test case to ensure the behavior is as expected.
| // Ensure the base URL ends with a slash so "models" is appended correctly | ||
| // Without this, new URL("models", "https://custom.requesty.ai/v1") would incorrectly | ||
| // resolve to "https://custom.requesty.ai/models" instead of "https://custom.requesty.ai/v1/models" | ||
| const baseWithSlash = resolvedBaseUrl.endsWith("/") ? resolvedBaseUrl : `${resolvedBaseUrl}/` |
Contributor
Author
There was a problem hiding this comment.
Consider extracting this URL construction logic into a small helper function for better testability and reusability. Something like:
Suggested change
| const baseWithSlash = resolvedBaseUrl.endsWith("/") ? resolvedBaseUrl : `${resolvedBaseUrl}/` | |
| const ensureTrailingSlashForPath = (url: string): string => { | |
| return url.endsWith("/") ? url : `${url}/` | |
| } | |
| const baseWithSlash = ensureTrailingSlashForPath(resolvedBaseUrl) |
| expect(mockAxios.get).toHaveBeenCalledWith("https://custom.requesty.ai/v1/models", { | ||
| headers: { Authorization: "Bearer test-api-key" }, | ||
| }) | ||
| }) |
Contributor
Author
There was a problem hiding this comment.
Great test coverage! Consider adding a few edge cases:
- URLs with query parameters (
?key=value) - URLs with fragments (
#section) - URLs with unusual port numbers (
:8080)
These would help ensure the URL construction is robust across all scenarios.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the broken URL construction for Requesty model fetching. The issue occurred when using custom base URLs with
/v1path - the models endpoint URL was incorrectly resolving to/modelsinstead of/v1/models.Problem
Given a custom URL like
https://custom.requesty.ai/v1, the model listing URL was incorrectly becominghttps://custom.requesty.ai/modelsinstead of the expectedhttps://custom.requesty.ai/v1/models.This happened because the
new URL()constructor replaces the last path segment when the base URL doesn't end with a slash.Solution
Testing
/v1, trailing slashes, and API key handlingFixes #7377
Important
Fixes URL construction in
getRequestyModelsto correctly append/modelsand adds comprehensive tests for various URL scenarios.getRequestyModelsinrequesty.tsto ensure base URL ends with a slash before appending/models./v1and trailing slashes.requesty.spec.tswith 10 new test cases for URL construction, including default/custom URLs and API key handling.requesty.tsexplaining URL construction logic.This description was created by
for 29174b3. You can customize this summary. It will automatically update as commits are pushed.