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

Fixed #14094 -- Added support for unlimited models.CharField on PostgreSQL. #16302

Merged
merged 1 commit into from Dec 28, 2022

Conversation

Elkasitu
Copy link

This commit introduced a new database feature flag
supports_unlimited_varchar which denotes which database backends
support defining VARCHAR columns with no/unlimited size.

When this feature flag is enabled for a given backend, CharField
definitions may rely on the default max_length value of None instead
of having to explicitly define one. It is however still possible to
explicitly pass a max_length and this will be enforced at the database
level.

This commit also implemented the necessary changes for the feature to
apply to the postgres backend.

@carltongibson
Copy link
Member

Just a query, on the ticket there was [a mention of inspectdb's output here]https://code.djangoproject.com/ticket/14094#comment:3). Did you happen to look into that at all?

@Elkasitu
Copy link
Author

@carltongibson I didn't, I wasn't aware of this Django feature until just now nor what Joe meant by "much smoother" (faster? less code complexity? etc.).

I have just tested it in a dummy Django project and the result is incorrect Django code (max_length=-1), I will take a look at it later today and see what I can do.

Thanks for the heads up!

@Elkasitu
Copy link
Author

Elkasitu commented Nov 22, 2022

@carltongibson it should work now, I am not exactly happy with the implementation (it assumes that a negative internal_size for a varchar column means that the varchar is of unlimited size, which works for pg but unsure about other backends) but I'm not sure that there's a better alternative given the way that inspectdb sets field attributes.

@Elkasitu
Copy link
Author

I could define a constant in the backend's instropection class e.g. UNLIMITED_VARCHAR_ISIZE = -1 (for pg) and use that in inspectdb to determine whether it is in fact a varchar of unlimited size

@Elkasitu
Copy link
Author

Rebased on top of the changes introduced by #16368, some tests are failing but the test that fails seems unrelated to my changes

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@Elkasitu Thanks for this patch 👍 Please check my comments.

django/core/management/commands/inspectdb.py Outdated Show resolved Hide resolved
docs/ref/models/fields.txt Outdated Show resolved Hide resolved
docs/releases/4.2.txt Outdated Show resolved Hide resolved
tests/inspectdb/models.py Outdated Show resolved Hide resolved
tests/inspectdb/tests.py Outdated Show resolved Hide resolved
django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved
django/db/backends/postgresql/base.py Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Dec 15, 2022

Also, a release note in the Models section is missing.

@Elkasitu Elkasitu force-pushed the unlimited-varchar branch 3 times, most recently from a04101e to ae0b972 Compare December 16, 2022 09:03
@Elkasitu
Copy link
Author

@felixxm I believe I've addressed all of your concerns now, there's one test failing but it seems like it fails randomly (previously it had failed on 3.11 and passed on 3.8)

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@Elkasitu Thanks 👍

@charettes Do you want to take a look?

@felixxm felixxm changed the title Refs #14094 -- Made max_length optional for backends that support it Fixed #14094 -- Added support for unlimited models.CharField on PostgreSQL. Dec 28, 2022
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@Elkasitu Thanks for updates 👍

I pushed small edits and fixed CharField.description.

django/core/management/commands/inspectdb.py Show resolved Hide resolved
@@ -340,6 +340,9 @@ class BaseDatabaseFeatures:
# Set to (exception, message) if null characters in text are disallowed.
prohibits_null_characters_in_text_exception = None

# Does the backend support unlimited VARCHAR columns?
supports_unlimited_varchar = False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
supports_unlimited_varchar = False
supports_unlimited_charfield = False

…eSQL.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants