-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(api): Add Pydantic schemas in OpenAPI annotations #19408
Conversation
Size Change: 0 B Total Size: 2 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
764a7c6
to
a1ab542
Compare
# Conflicts: # posthog/api/services/query.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 👍
Looks good to my untrained eye. I noticed some weirdness in one new generated Python class, but nothing blocking if you think this is ready to go.
frontend/src/queries/schema.ts
Outdated
// Dynamically make a union type out of all the "response" fields in QuerySchema | ||
type ResponseType<T> = T extends { response: infer R } ? R : never | ||
type AllResponses = ResponseType<QuerySchema> | ||
type Unionize<T> = Partial<{ | ||
[P in keyof T]: T[P] | ||
}> | ||
export type QueryCombinedResponse = Unionize<AllResponses> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat!
# query_json has been parsed by QuerySchemaParser | ||
# it _should_ be impossible to end up in here with a "bad" query | ||
query_kind = query_json.get("kind") | ||
tag_queries(query=query_json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariusandra not sure what this does, and I don't have a dict anymore that I can pass to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a copy of the input query into ClickHouse's system.query_log
table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what can we do? I don't want to serialize again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tag the queries where I still have the dict.
# Conflicts: # frontend/src/queries/schema.ts
Trying again #19408 with fixes
Problem
We don't or can't use our Pydantic models as schemas in our OpenAPI spec.
We also don't use discriminate unions to have the model validation work better.
Changes
isinstance
doesn't work on type aliases that use a union python/mypy#11673request.data
as the Pydantic model, but everything in Django/DRF expectsrequest.data
to be a dict, so that's a no-goHow did you test this code?