URLFor class to generate URLs at serialisation time#242
Merged
Conversation
Barecheck - Code coverage reportTotal: 96.33%Your code coverage diff: 0.07% ▴ Uncovered files and lines
|
Contributor
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
I'm fine with the code changes in isolation. I don't full understand the context of when url_for is called though
julianstirling
approved these changes
Jan 10, 2026
Contributor
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
One final question on why the Pydantic Style class exists. I assume it is made before the context where it is used is set, but I don't understand where that would be.
Are you able to add a bit more context to the top of the class docstring before merging?
This adds: * a middleware function that pushes the `url_for` method of the `Request` object to a context variable. * a `pydantic`-compatible class that calls `url_for` to generate URLs at serialisation time. * a test harness that allows this to be used outside of an HTTP request handler. The middleware is tested on its own and in the context of a FastAPI app, but isolated from the rest of LabThings.
Verify that both `URLFor` serialisation and `url_for` are OK in request handlers, but the latter fails in a thread. This should help make it clear where you can (and can't) use the two ways of getting URLs.
Make it clearer why URLFor exists and when it's appropriate to call `url_for`.
rwb27
force-pushed
the
url-for-middleware
branch
from
4741776 to
3ec2b8b
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
There are a number of places in LabThings where we must generate URLs pointing to the LabThings server. This is done in several ways, none of which are very nice. This PR lays the groundwork for improving URL generation, especially:
BlobsInvocationsOnce it's in, it should make it relatively straightforward to eliminate things like
BlobManagerContextDepand the need to pass paths around the application as strings that may or may not be consistent with the actual endpoint URLs.This PR adds:
url_formethod of theRequestobject to a context variable.pydantic-compatible class that callsurl_forto generate URLs at serialisation time.The middleware is currently tested on its own and in the context of a FastAPI app, but isolated from the rest of LabThings.
The middleware isn't currently added to the app created by the
ThingServer. I might describe that in a separate PR that also uses it to tidy up e.g. blob serialisation: it may simplify review to split the two, though probably it is easier to review this PR once I've made the next one that actually uses it.