Make use of url_for_middleware to tidy up Blob and Invocation URLs
#244
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
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.
This commit makes use of the new `url_for` middleware to eliminate the Blob-specific context variables. BlobData objects are now added to a singleton BlobManager when they are created, and the URL is filled in at serialisation time. This is a slight simplification of the old behaviour, but it's equivalent in all the ways that matter.
Having now learned more about custom types in pydantic, I've done some more tidying here: * Blob is no longer a BaseModel subclass. I've separated out the model (used for serialisation/validation) and the class that user code will interact with. * BlobData is now a base class not a protocol, and there's a subclass for remote blob data that downloads on demand. This removes most of the complicated logic from `Blob` around when we do and don't need a `BlobData`: a `Blob` is **always** backed by `BlobData` whether it's local or remote. This also means we can get rid of `ClientBlobOutput` and just use `Blob` instead.
This now correctly tells clients the media type, and uses a descriptive title. I believe it's now at least as good as the old schema.
We can now use one Blob class for client and server :) I realised we had the potential to have inconsistencies between BlobData and the host Blob in the media type. We now check the types match, and allow the BlobData to override the Blob's default if it's a matching but more specific type. I've also take a pass through the blob documentation to update it where needed. Happily, as this PR only touches implementation details, not much has changed.
I got rid of the conversion of "*" to None, I think it's clearer this way. I also fixed a typo and ignored a codespell false positive.
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.
This PR makes use of the changes in #242 to serialise
Blobs more neatly. It contains those commits and should be merged afterwards.I refactored
Blobmore significantly, making a number of under-the-hood changes that I think make it much clearer. One of the early commits includes a working version ofBlobthat usesurl_forbut hasn't yet been refactored. However, I don't think there's much point reviewing two versions of the same thing, hence not splitting the PR.Blobis no longer aBaseModelsubclass. Instead, it's a regular class that also works as apydantictype, just likeURLFor.Blobmay now refer to local (bytesor file) or remote (URL) data, withBlobDatasubclasses for each.ClientBlobOutputis gone, we can useBlobinstead. That eliminates a whole module and is a step towards client and server types matching.Protocolin favour of base classes - I don't see many (if any) reasons to add newBlobDatatypes, and if we do I don't see any reason they can't also inherit fromBlobData.Blobobjects now serialise properly when returned from any endpoint in the server. Serialising them in other contexts requires the use oflabthings_fastapi.testing.use_dummy_url_for.I've also removed the need to pass
requesttoInvocations when generating their response - we can useURLForto generate the URLs that need to show up in the output.This ended up being a larger change than I intended, but I think it results in a cleaner structure, and provides
pydanticfunctionality in a way that doesn't mess up the structure of classes that client code needs to use.