-
Notifications
You must be signed in to change notification settings - Fork 5
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
[#38] code migration to use pydantic v2 #51
base: develop
Are you sure you want to change the base?
Conversation
…into 38-update-to-pydantic-v2
…scheduler to be upgraded
…38-update-to-pydantic-v2
|
||
import typer | ||
from apscheduler.events import EVENT_JOB_ERROR, EVENT_JOB_EXECUTED |
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.
Had to replace rocketry
with apscheduler
as rocketry
has a dependency on pydantic
. Currently rocketry
is using pydantic
v1.
api/adapters/hydroshare.py
Outdated
funder.name = self.funding_agency_name | ||
if self.funding_agency_url: | ||
funder.url = self.funding_agency_url | ||
funder.url = str(self.funding_agency_url) |
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.
Should the Organization.funder.url attribute be updated to an HttpUrl instead of casting to a string here?
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.
The funder url has been updated to type HttpUrl:
https://github.com/I-GUIDE/catalogapi/blob/38-update-to-pydantic-v2/api/adapters/hydroshare.py#L53
api/config/__init__.py
Outdated
@@ -24,13 +25,14 @@ def __init__(self, **data: Any) -> None: | |||
super().__init__(**data) | |||
if self.testing: | |||
self.database_name = f"{self.database_name}" | |||
self.hydroshare_meta_read_url = str(self.hydroshare_meta_read_url) | |||
self.hydroshare_file_read_url = str(self.hydroshare_file_read_url) |
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.
hmm, there must be something else going on with the HttpUrl->str conversions that I'm not understanding. Why are these lines in here?
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 also was a big headche for me when doing this upgrade. See this pydantic discussion where it talks about the problem with pydantic HttpUrl type.
pydantic/pydantic#6395
|
||
@property | ||
def db_connection_string(self): | ||
return f"{self.db_protocol}://{self.db_username}:{self.db_password}@{self.db_host}/?retryWrites=true&w=majority" | ||
|
||
class Config: | ||
env_file = ".env" | ||
model_config = SettingsConfigDict(env_file=".env", env_file_encoding="utf-8") |
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.
Just noting that I have since learned that if we leave the env_file Config out then it just reads environment variables which eases deployments. This comment is not relevant to these changes, just making a note here.
api/models/schema.py
Outdated
# two possible types - one of which is null (due to the field being optional) | ||
json_schema = handler(core_schema) | ||
json_schema = handler.resolve_ref_schema(json_schema) | ||
for field in ("url", "description"): |
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.
These one off json schema modifiers look hairy. Any way we can consolidate these changes to the base class and make the changes for certain types of attributes?
api/models/user.py
Outdated
@model_validator(mode='after') | ||
def url_to_string(self): | ||
if self.url is not None: | ||
self.url = str(self.url) |
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'm missing something on these httpurl to str conversions. Why is this change needed?
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.
It mostly looks good. I just have the question around the httpurl->str conversions and also would like to revisite the json schema modifications to see if we can make the changes more general by type instead of having custom implementations in each class.
No description provided.