Better errors for bad return values#333
Open
rwb27 wants to merge 19 commits into
Open
Conversation
rwb27
force-pushed
the
validate-return-values
branch
from
9c256a9 to
e945edc
Compare
Barecheck - Code coverage reportTotal: 96.81%Your code coverage diff: -0.19% ▾ Uncovered files and lines |
rwb27
force-pushed
the
validate-return-values
branch
from
e945edc to
1a60cdd
Compare
Collaborator
Author
|
This branch has fallen foul of separate input and output schemas in Pydantic. That seems to mean the OpenAPI docs are missing schemas for GET requests to properties. Disabling schema-splitting fixes the problem, so I'll likely include that in this PR. |
rwb27
force-pushed
the
validate-return-values
branch
from
55f5a21 to
26852c6
Compare
bprobert97
reviewed
May 21, 2026
rwb27
force-pushed
the
validate-return-values
branch
from
abab82d to
452f493
Compare
Action outputs are serialized using a pydantic model. We now create (and validate) this model instance in the action thread, rather than in the response handler returning it to the user. This means that validation errors will now be caught and logged, and there won't be loads of ASGI/routing/FastAPI stuff in the stack trace. It does not yet fix the problem of un-JSONable types being returned, because `pydantic` will allow `Any` to be wrapped in a `RootModel` which validates fine, and fails at serialisation time.
The stack trace is not useful when an action returns an invalid value, as it only tells us about LabThings code. The error is still logged, but there's no stack trace.
This commit: * Adds an error handler for PydanticSerializationError * Fixes a race condition in invocation responses that could result in an output being present before the response marks the invocation as complete * Handles HTTP errors when polling actions in ThingClient * Ensures that invocation models attach useful debug info to serialization errors (they add the invocation ID and action path).
Depending on whether the action fails before or after the initial response is sent to the client, we get a different exception (with the same message). This commit will accept either kind of failure so long as it includes the same message. This should prevent intermittent test failures: there's nothing to synchronise the action thread and the HTTP response, and it's not feasible to add that now.
This commit moves `wrap_plain_types_in_rootmodel` into the `RootModelWrapper` class as a class method, and adds better error handling. This now returns class and attribute references, or file and line number in the case of functions. Currently these errors are raised if unserializable types are declared. A future commit will raise similar errors at runtime if unserialisable values are found.
This commit moves both validation and serialisation of properties into LabThings code, so the exception may be handled properly. If this looks good, we may want to make a custom Response class.
This was causing properties to have no schema for their GET endpoint. It's detailed at https://fastapi.tiangolo.com/how-to/separate-openapi-schemas/#same-schema-for-input-and-output-models-in-docs
This commit removes the complicated wrapper-based error handling in favour of a function. This approach is specific (only catches ValidationError/SerializationError directly related to user code) and avoids our custom errors being re-wrapped by `pydantic`.
This makes a few changes to ensure we handle serialization errors better: 1. Invocations now only return their input, output, log, and links when accessed individually. This avoids the possibility of serialization errors in lists of invocations (and should give a decent performance boost). 2. Endpoints that return values from downstream code (property reads or action outputs) now return `Response` directly, and use `serialize_from_user_code` to apply sensible error handling. 3. Everywhere I've used `serialize_from_user_code` I've specified the model in the FastAPI decorator, and ensured the exception is handled, logged, and re-raised as an HTTPException. I've split up `response_from_user_code` to handle validation and serialization separately: that fits better with the way actions are structured.
These may be used by some clients to find the full model, so they're useful here.
This checks that we get the correct errors when accessing bad values over HTTP. Note that an invocation with an unserializable return value will raise an error if it is accessed individually, but won't cause the global endpoints to fail.
From @bprobert97's review
This codebase now consistently uses the `-ise` ending,
There's now code in the exceptions module that needs testing: this is now done. The code is also simplified to remove branching, using one form that works regardless of how many arguments there are.
There's now a test for the initialiser of FunctionalProperty. This revealed that there was a spurious check for missing return types: we already substitute `Any` in this case. I also fixed a format issue with a type hint that was getting split over multiple lines.
There's a concurrency issue that means sometimes an action failed to invoke, and other times it invoked but failed to complete - that gave two slightly different errors. The test should now pass for either.
The error handling code in poll_invocation now has a couple of explicit tests, which make sure it does the right thing if the error is not in the expected format.
bprobert97
reviewed
May 29, 2026
Contributor
bprobert97
left a comment
bprobert97
left a comment
There was a problem hiding this comment.
I have replied to the discussion in the one open thread
rwb27
force-pushed
the
validate-return-values
branch
2 times, most recently
from
ea1f4b5 to
704239c
Compare
Exception.args is a tuple (at least according to mypy) so we needn't worry about `None`. This now handles correctly: * An empty tuple (no arguments) * An empty string * A non-empty string * A tuple with multiple elements * A tuple where the first element isn't a string.
rwb27
force-pushed
the
validate-return-values
branch
from
704239c to
152f33e
Compare
bprobert97
approved these changes
Jun 1, 2026
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 quite a few places in the codebase where values that come from downstream code are returned over HTTP. As a rule, these values are returned from endpoint functions, and FastAPI then attempts to serialise them using
pydantic. If the value cannot be serialized or validated by the model (because we need a model instance to serialize from, and we can only get one by validating the input data), we get an ugly error. "ugly" in this case means there's an enormous stack trace, almost all of which refers tofastapiorstarletteorpydanticand none of which tells you where in the code you actually wrote the problem arose.This merge request attempts to address that problem, by taking control of how values are serialized. This applies to:
The intention of this PR is not to change the current behaviour when there's no error. However, I've had to duplicate a small amount of FastAPI's logic in LabThings so we can handle the resulting errors safely. These changes should be moving validation/serialisation logic that was already happening implicitly in FastAPI into our codebase: I don't believe I've added extra validation or serialisation steps.
Changes include:
response_modelspecified (or to functions typed as returning a model that actually return a simple value, but there shouldn't be any of those left). This is particularly relevant to property getters.Responseinstead of returning a model instance (or value to be validated into a model instance). This allows us to catch serialization errors and provide a useful message pointing to the origin of the un-serialisable value.RootModelWrapperis refactored into theRootModelWrapperclass. The functionality should be identical.RootModelWrapper. I couldn't find an obvious cause, but I think it may be to do with specifying a default value. I've set a FastAPI option not to split input and output schemas, which fixes the problem. This ought not to be a change anybody notices.I've added some helper functions to keep this neat, and also to ensure we provide useful hints in the error message as to where in the downstream code the problem may have occurred - for example, actions will give the name of the action, and the file and line number where the function was defined. Getting line numbers for properties is harder, so for now we just provide a link to the class and the name of the property.
All together, these changes mean that, when a bad value is passed back to a client, we respond with a helpful
500HTTP error response and log a useful error on the server.There are more places in LabThings where error handling could be improved: the biggest one I can think of is the thing description, where bad types or bad default values can lead to hard-to-debug errors. I have deliberately not swallowed all validation/serialization errors because that's likely to hide bugs. This PR just hides the ones for which we can obviously point to a source in user code.
Closes #270
Closes #338