Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct JSON schema dialect for param validation #15483

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

GfxKai
Copy link
Contributor

@GfxKai GfxKai commented Sep 24, 2024

Closes #13560

Whilst this issue is already closed, the problem is not actually fixed in v3, see #13560 (comment)

Context

This issue occurs when trying to deploy a flow which uses a Pydantic model as a parameter. Specifically, JSON schema validation for this parameter fails if the model contains a field with a numeric gt or lt constraint (or a type alias to that effect, e.g. pydantic.PositiveFloat).

Cause

  1. On the client side, prefect.deployments.runner.RunnerDeployment calls prefect.utilities.callables.generate_parameter_schema to generate a JSON schema for the flow parameters

  2. Internally, this calls prefect._internal.pydantic.v2_schema.create_v2_schema, which uses a subclass of pydantic.json_schema.GenerateJsonSchema for the actual schema generation

Crucially, this class (and Pydantic in general since since v2.0) uses Draft 2020-12 as the default JSON Schema dialect.

  1. When Prefect server receives the POST /api/deployments/ request, FastAPI tries to construct and validate a prefect.server.schemas.actions.DeploymentCreate model from the request body

  2. The model validation method for DeploymentCreate validates the parameter OpenAPI schema using prefect._internal.schemas.validators.validate_parameter_openapi_schema

  3. Internally, this calls jsonschema.Draft4Validator.check_schema to do the actual validation

Obviously, this schema validation class uses the Draft 4 JSON Schema dialect.

As you can see in the JSON Schema docs, exclusiveMinimum and exclusiveMaximum were boolean as of Draft 4 but have subsequently changed to numeric type.

Proposed solution

Update the server-side validation to use jsonschema.Draft202012Validator

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

@github-actions github-actions bot added the bug Something isn't working label Sep 24, 2024
Copy link

codspeed-hq bot commented Sep 24, 2024

CodSpeed Performance Report

Merging #15483 will not alter performance

Comparing GfxKai:patch-1 (acd7244) with main (3cdcf87)

Summary

✅ 3 untouched benchmarks

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the PR @GfxKai - nice catch!

@zzstoatzz zzstoatzz added great writeup This is a wonderful example of our standards fix A fix for a bug in an existing feature labels Sep 24, 2024
@zzstoatzz zzstoatzz merged commit 9eed6ad into PrefectHQ:main Sep 24, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix A fix for a bug in an existing feature great writeup This is a wonderful example of our standards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model fields that use gt generate invalid JSON schema
2 participants