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

Fix response typing in OpenAPI schema for GlobalConcurrencyLimitResponse #16297

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

desertaxle
Copy link
Member

@desertaxle desertaxle commented Dec 9, 2024

The generated client for the v2 UI has been showing id, created, updated as optional for a response from the API, even though they should always be present. I tracked this down to the response models inheriting from ORMBaseModel, which has default factories for these three fields. The default factories will cause these fields to appear optional in the generated OpenAPI schema.

This PR introduces a ResponseBaseModel with id, created, and updated as required attributes and uses this new model to update GlobalConcurrencyLimitResponse as a test. The generated code now reflects the expected typing.

I started with one model to ensure that we like this pattern. If we do, I can update other response models in followup PRs.

Copy link

codspeed-hq bot commented Dec 9, 2024

CodSpeed Performance Report

Merging #16297 will not alter performance

Comparing fix-server-response-typing (44d3b68) with main (612106c)

Summary

✅ 3 untouched benchmarks

@desertaxle desertaxle force-pushed the fix-server-response-typing branch from c7fb38f to 520d28b Compare December 10, 2024 02:21
@github-actions github-actions bot added the docs label Dec 10, 2024
@desertaxle desertaxle changed the title Fix response typing in OpenAPI schema Fix response typing in OpenAPI schema for GlobalConcurrencyLimitResponse Dec 10, 2024
@desertaxle desertaxle marked this pull request as ready for review December 10, 2024 16:15
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.

lgtm!

@zzstoatzz
Copy link
Collaborator

zzstoatzz commented Dec 10, 2024

lgtm!

actually I just remembered, @desertaxle what would be the difference between IDBaseModel and ObjectBaseModel and this?

would it just be roughly the same model but for responses instead?

@desertaxle
Copy link
Member Author

actually I just remembered, @desertaxle what would be the difference between IDBaseModel and ObjectBaseModel and this?

would it just be roughly the same model but for responses instead?

Yes, ResponseBaseModel would be a server-side only model that doesn't include any default_factory values to ensure the OpenAPI schema generates with id, created, and updated as required instead of optional.

@zzstoatzz
Copy link
Collaborator

actually I just remembered, @desertaxle what would be the difference between IDBaseModel and ObjectBaseModel and this?
would it just be roughly the same model but for responses instead?

Yes, ResponseBaseModel would be a server-side only model that doesn't include any default_factory values to ensure the OpenAPI schema generates with id, created, and updated as required instead of optional.

cool sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants