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: Embed ResponseModel converter middleware in service API #1804

Merged
merged 11 commits into from
Jan 12, 2024

Conversation

fregataa
Copy link
Member

@fregataa fregataa commented Dec 21, 2023

follow-up #1752

Currently we have check_api_params_v2 decorator function which check request's parameters and parse response before return it. But there are some API functions that isn't decorated by it and just returns ResponseModel, which occurs error. So,

  • Embed a webapp middleware to parse the response of the functions.

Plus, fix some pydantic APIs usage when parse ResponseModel to web response.

  • Change TypeAdapter.dump_json() to TypeAdapter.dump_python() because dump_json() returns serialized bytes.
  • Change BaseModel.model_dump_json() to BaseModel.model_dump() because model_dump_json() returns JSON string, not dict.

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

Sorry, something went wrong.

@fregataa fregataa added this to the 24.03 milestone Dec 21, 2023
@fregataa fregataa self-assigned this Dec 21, 2023
@github-actions github-actions bot added comp:manager Related to Manager component size:M 30~100 LoC labels Dec 21, 2023
@fregataa fregataa requested a review from lizable December 21, 2023 09:44
@fregataa fregataa added the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Dec 21, 2023
Copy link
Contributor

@lizable lizable left a comment

Choose a reason for hiding this comment

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

I think returning pydantic instance for only those functions with check_api_params_v2 decorators would be enough. I'm not sure we really need middleware for this issue. How about just reverting response type and value of delete function instead of adding middleware for converting responses?

I reverted and tried deleting service model and It worked fine.
https://github.com/lablup/backend.ai/pull/1752/files#diff-d21fbe306a9b579a2491f344fc4de5eff8407147aac0c18568f4ca95880b27c2L409-R577

@github-actions github-actions bot added size:S 10~30 LoC and removed size:M 30~100 LoC labels Jan 2, 2024
@fregataa fregataa changed the title fix: Embed ResponseModel converter middleware fix: Return web_response.StreamResponse as is Jan 2, 2024
@fregataa fregataa changed the title fix: Return web_response.StreamResponse as is fix: Embed ResponseModel converter middleware in service API Jan 2, 2024
This reverts commit 1dd2c33.
@github-actions github-actions bot added size:M 30~100 LoC and removed size:S 10~30 LoC labels Jan 2, 2024
@fregataa
Copy link
Member Author

fregataa commented Jan 2, 2024

I think returning pydantic instance for only those functions with check_api_params_v2 decorators would be enough. I'm not sure we really need middleware for this issue. How about just reverting response type and value of delete function instead of adding middleware for converting responses?

I reverted and tried deleting service model and It worked fine. https://github.com/lablup/backend.ai/pull/1752/files#diff-d21fbe306a9b579a2491f344fc4de5eff8407147aac0c18568f4ca95880b27c2L409-R577

Thank you for reviewing. But this is a good way to use a middleware because all service APIs should be filtered by a certain function and this is not a global middleware.

@fregataa fregataa requested a review from lizable January 2, 2024 09:35
@kyujin-cho kyujin-cho enabled auto-merge January 12, 2024 13:34
@kyujin-cho kyujin-cho added this pull request to the merge queue Jan 12, 2024
Merged via the queue into main with commit 7668404 Jan 12, 2024
25 checks passed
@kyujin-cho kyujin-cho deleted the fix/convert-model-to-response branch January 12, 2024 13:40
kyujin-cho added a commit that referenced this pull request Feb 21, 2024
Co-authored-by: Kyujin Cho <kyujin.cho@lablup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component size:M 30~100 LoC urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants