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.

Sign up for GitHub

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

Jump to bottom

SAWarning in cache key for CIText() #25

Open
miketheman opened this issue Jan 2, 2022 · 5 comments · Fixed by akolov/sqlalchemy-citext#2 · May be fixed by #28
Open

SAWarning in cache key for CIText() #25

miketheman opened this issue Jan 2, 2022 · 5 comments · Fixed by akolov/sqlalchemy-citext#2 · May be fixed by #28

Comments

@miketheman
Copy link

Observed using SQLAlchmey 1.4.28 and sqlalchemy-citext 1.8

SAWarning: UserDefinedType CIText() will not produce a cache key because the cache_ok attribute is not set to True. This can have significant performance implications including some performance degradations in comparison to prior SQLAlchemy versions. Set this attribute to True if this type object's state is safe to use in a cache key, or False to disable this warning. (Background on this error at: https://sqlalche.me/e/14/cprf)

Introduced in SQLAlchemy sqlalchemy/sqlalchemy@22deafe

I'm not entirely certain how this would affect behaviors, but wanted to surface this.
miketheman added a commit to miketheman/warehouse that referenced this issue Jan 4, 2022
@miketheman
test: ignore CIText warnings in test output
bf0b516 
The updated SQLAlchemy library now emits 2,000+ warnings for CIText().

I've opened an issue on the responsible repo.

Refs: mahmoudimus/sqlalchemy-citext#25

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
di added a commit to pypi/warehouse that referenced this issue Jan 4, 2022
@miketheman @di
chore: handle more warnings in test suite (#10547)
af038f5 
* chore: replace deprecated scalar method

SQLAlchemy 1.4.x deprecated the `.as_scalar()` method and replaced it
with `.scalar_subquery()` and issued a `SADeprecationWarning`.

Refs: https://docs.sqlalchemy.org/en/14/changelog/changelog_14.html#change-f413ff8c13a1fb7d766bfa16dfcdfbf1

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: ignore InsecureStorageWarning

When another class was added, it wasn't excluded from the warnings
report.

Refs: #9792
Original ignore: #4360

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: ignore CIText warnings in test output

The updated SQLAlchemy library now emits 2,000+ warnings for CIText().

I've opened an issue on the responsible repo.

Refs: mahmoudimus/sqlalchemy-citext#25

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: filter out known legacy behavior in test

The test case knowingly creates invalid versions, and the code handles
them by performing some extra validation, and throws the error.

Suppress the warnings for creating `LegacyVersion`.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: filter out SAWarning for invalidation

The collections used in the test case are intentionally being modified.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
@HansBouwmeester
Copy link

Same issue. So is it ok to tell SQLAlchemy cache_ok=True?

@TimelyToga
Copy link

@Gra55h0pper my understanding is that since this is a stateless type definition then we are good to set cache_ok = True

evrys added a commit to evrys/sqlalchemy-citext that referenced this issue Jun 1, 2022
@evrys
Set cache_ok=True for CIText type
02e37ed 
Fixes mahmoudimus#25, an SAWarning when performing queries with CIText columns
@evrys
Copy link

evrys commented Jun 1, 2022

Sent a PR to add cache_ok = True, in meantime can work around it by extending the class like so

class CacheyCIText(CIText):
    # Workaround for https://github.com/mahmoudimus/sqlalchemy-citext/issues/25
    # Can remove when that issue is fixed
    cache_ok = True

@mahmoudimus
Copy link
Owner

mahmoudimus commented Jun 1, 2022 •
edited
Loading

Will take a look and push it out in 24 hours if it works. Thanks for the PR.

rouge8 added a commit to rouge8/sqlalchemy-citext that referenced this issue Jun 2, 2022
@rouge8
Support caching CIText in SQLAlchemy's SQL compilation caching
14e14c5 
Fixes mahmoudimus#25.
@rouge8 rouge8 linked a pull request Jun 2, 2022 that will close this issue
Open
@rouge8
Copy link

rouge8 commented Jun 2, 2022

#28

domdfcoding pushed a commit to domdfcoding/warehouse that referenced this issue Jun 7, 2022
@miketheman @di @domdfcoding
chore: handle more warnings in test suite (pypi#10547)
d89d523 
* chore: replace deprecated scalar method

SQLAlchemy 1.4.x deprecated the `.as_scalar()` method and replaced it
with `.scalar_subquery()` and issued a `SADeprecationWarning`.

Refs: https://docs.sqlalchemy.org/en/14/changelog/changelog_14.html#change-f413ff8c13a1fb7d766bfa16dfcdfbf1

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: ignore InsecureStorageWarning

When another class was added, it wasn't excluded from the warnings
report.

Refs: pypi#9792
Original ignore: pypi#4360

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: ignore CIText warnings in test output

The updated SQLAlchemy library now emits 2,000+ warnings for CIText().

I've opened an issue on the responsible repo.

Refs: mahmoudimus/sqlalchemy-citext#25

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: filter out known legacy behavior in test

The test case knowingly creates invalid versions, and the code handles
them by performing some extra validation, and throws the error.

Suppress the warnings for creating `LegacyVersion`.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* test: filter out SAWarning for invalidation

The collections used in the test case are intentionally being modified.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
@rouge8 rouge8 mentioned this issue Jul 6, 2022
Closed
@akolov akolov mentioned this issue Oct 11, 2022
Merged
akolov added a commit to akolov/sqlalchemy-citext that referenced this issue Oct 11, 2022
@akolov
Merge pull request #2 from rouge8/mahmoudimusgh-25
15b3de8 
Support caching `CIText` in SQLAlchemy's SQL compilation caching
@cjmayo cjmayo mentioned this issue Nov 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@mahmoudimus @rouge8 @miketheman @TimelyToga @HansBouwmeester @evrys