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

Correct imports and improve generation logic in SQLModelGenerator #352

Closed
wants to merge 4 commits into from

Conversation

touale
Copy link

@touale touale commented Dec 3, 2024

In fact, I found that there are some problems in the code generated by sqlacodegen for sqlmodel, which is manifested in the introduction of redundant class CHAR and the mapped_column does not been import. The following is a detailed description of the problem:

image

(1) When encountering CHAR(36) and CHAR(36, 'utf8mb4_general_ci'), sqlacodegen will repeatedly import from different sources

from sqlalchemy import CHAR
from sqlalchemy.dialects.mysql import CHAR

The adjustment I made was to focus on sorted and high-priority imports, and to make a warn about duplicate imports.

for example:

WARN: Duplicate imports `{'CHAR'}`  are detected from the package `sqlalchemy.dialects.mysql` and will be filtered, 
which may cause abnormal behavior.

In fact, "from sqlalchemy import CHAR" supports general projects of multiple databases, while "from sqlalchemy.dialects.mysql import CHAR" is specifically for MySQL projects. If you project requires cross-database compatibility or you are not sure about the target database type, it is safer to use "sqlalchemy.CHAR".

(2) sqlmodel does not support mapped_column in the latest release(https://github.com/fastapi/sqlmodel/releases/tag/0.0.22), although someone has proposed a PR (fastapi/sqlmodel#1143), so Column should be used for the generation of sqlmodels for the time being

@agronholm agronholm added this to the 3.0 milestone Dec 28, 2024
@sheinbergon sheinbergon self-requested a review January 9, 2025 22:27
@sheinbergon
Copy link
Collaborator

@agronholm I believe #358 also takes care of this PR, at least the mapped_column (and albit in a cleaner fashion). the CHAR part, is still an issue I guess. Once the former is merged, we can revisit this one

@touale
Copy link
Author

touale commented Jan 11, 2025

In fact, the changes I made were aimed at quickly and efficiently resolving my production and development issues in a short period of time

@agronholm
Copy link
Owner

This is a quick and dirty patch I will not accept. @sheinbergon your PR is the way to go. What in particular I don't like here is that the superclass is required to know about the subclass which is bad design.

@agronholm agronholm closed this Jan 11, 2025
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.

3 participants