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

Making checks pass #1

Merged
merged 48 commits into from
Sep 18, 2023
Merged

Making checks pass #1

merged 48 commits into from
Sep 18, 2023

Conversation

mbsantiago
Copy link

@mbsantiago mbsantiago commented Aug 9, 2023

Hi @AntonDeMeester,

Thank you for your work on updating sqlmodels to pydantic2 and sqlalchemy2. I'm excited about the possibility of having this pull request merged.

I've gone ahead and forked it to make sure that all checks are passing smoothly. Here's a brief overview of the adjustments I've made:

  1. FastAPI Tutorial Tests: I've reviewed the FastAPI tutorial tests and made sure they are now passing. It appears that they were issues due to some changes in the OpenAPI schema. Specifically, the schema version required updating, and there were adjustments required in the properties definition for nullable fields.

  2. Validation Test: One of the tests related to validation was failing. This test was aimed at ensuring that SQLModel remains aligned with Pydantic by not executing validation on fields with default values. I've made some modifications to the SQLModel.validate_model method to ensure that default values are exempt from validation.

  3. Mypy Fixes: I've addressed six mypy errors. These errors seem minor, but they might need review to ensure there are no typing issues introduced. Two errors were associated with the override of the query and execute methods within the Session class. These overrides were primarily intended for altering docstrings, so I've updated the overrides to exclusively target the docstrings.

I've run both the lint.sh and test.sh scripts, and they are passing in my computer. Hopefully this will help to get the pull request merged.

Cheers,
Santiago

@AntonDeMeester
Copy link
Owner

Hey Santiago. It makes sense. One thing though, we're making the Open API documentation as optional. While it's theoretically true, it does seem a bit weird, as it would provide DB errors.
But I think that's something that the SQL Model maintainers need to figure out themselves how to deal with that.

@AntonDeMeester AntonDeMeester self-requested a review August 9, 2023 07:24
Copy link
Owner

@AntonDeMeester AntonDeMeester left a comment

Choose a reason for hiding this comment

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

Looks really good!

Comment on lines +581 to +583
hasattr(field, "default")
and field.default is not PydanticUndefined
and value == field.default
Copy link
Owner

Choose a reason for hiding this comment

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

We. need to figure out default_factory as well I think.

Suggested change
hasattr(field, "default")
and field.default is not PydanticUndefined
and value == field.default
hasattr(field, "default")
and field.default is not PydanticUndefined
and field.default_factory is not None
and value == field.default

Copy link
Author

Choose a reason for hiding this comment

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

The default factory is a bit trickier since it is hard to know if the passed value is the output of the default factory or a user provided value. I'm not sure if Pydantic does run field validations in the case it was generated by the default factor, but if it doesn't then we need to avoid SQLModel from running the validation.

@@ -152,3 +100,34 @@ def get(
execution_options=execution_options,
bind_arguments=bind_arguments,
)


Session.query.__doc__ = """
Copy link
Owner

Choose a reason for hiding this comment

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

I like it!

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this will impact doc generation though. Didn't manage to run the make docs script locally, so I was waiting for it to run on GitHub to verify.

"age": {"title": "Age", "type": "integer"},
"age": {
"title": "Age",
"anyOf": [{"type": "integer"}, {"type": "null"}],
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if this is intended, as the real DB model is not nullable often. TBD

Copy link
Author

Choose a reason for hiding this comment

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

I see. I believe this is caused by Pydantic v1 confusion between nullable and optional fields (see changes-to-json-schema-generation and required-optional-and-nullable-fields). I think it makes sense to align to Pydantic v2 way of specifying nullable and optional fields.

@mbsantiago
Copy link
Author

Hey @AntonDeMeester! I hadn't checked if the tests passed on other python versions. I've made the necessary changes in an additional commit and I've run them using GitHub actions to have the same testing workflow. It seems to be all green now.

@mbsantiago
Copy link
Author

The make_docs workflow is failing. Seems to be an incompatibility with importlib-metadata version. I added it to poetry's dev-dependencies so it can install a version compatible with mkdocs.

@mbsantiago
Copy link
Author

Hi @AntonDeMeester! It seems that all checks are passing. Could you approve the workflows to check?

@AntonDeMeester
Copy link
Owner

Will do! I can merge it but it's still up to the brilliant creator of SQL Model to approve the merge.

@AntonDeMeester AntonDeMeester merged commit 244c947 into AntonDeMeester:main Sep 18, 2023
8 of 10 checks passed
AntonDeMeester pushed a commit that referenced this pull request Oct 12, 2023
* Updating version for mkdocs to fix failing test
AntonDeMeester added a commit that referenced this pull request Oct 12, 2023
Updating version for mkdocs to fix failing test (#1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants