Better errors for bad return values#333
Open
rwb27 wants to merge 12 commits into
Open
Conversation
rwb27
force-pushed
the
validate-return-values
branch
from
9c256a9 to
e945edc
Compare
Barecheck - Code coverage reportTotal: 96.45%Your code coverage diff: -0.41% ▾ Uncovered files and lines |
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.
rwb27
force-pushed
the
validate-return-values
branch
from
e945edc to
1a60cdd
Compare
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.
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. |
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.
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
1 participant
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 first change is that we now instantiate models ourselves, rather than relying on FastAPI to do it. This ensures we can catch and handle any validation errors.
The second change is that we now serialize those models to JSON and return a
Responseinstead of returning a model instance. This allows us to catch serialization errors.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.
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, just the ones for which we can obviously point to a cause.