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
rwb27
force-pushed
the
refactor-blob
branch
2 times, most recently
from
18da201 to
ba1e4b3
Compare
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.
The `codespell:ignore` directives made lines too long, and I'm happy that "ser" is an abbreviation we are stuck with.
Barecheck - Code coverage reportTotal: 96.52%Your code coverage diff: 0.22% ▴ |
This gets full coverage of `blob.py` and checks a few things that weren't tested directly. I actually don't quite understand why coverage thought we weren't downloading the data as I'm fairly sure that was done already - but it's no bad thing to have an explicit test that doesn't go via the ThingServer.
This adds a sequence of events for blob serialisation,
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.