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

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
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
* 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
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

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
Copy link

rouge8 commented Jun 2, 2022

#28

domdfcoding pushed a commit to domdfcoding/warehouse that referenced this issue Jun 7, 2022
* 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>
akolov added a commit to akolov/sqlalchemy-citext that referenced this issue Oct 11, 2022
Support caching `CIText` in SQLAlchemy's SQL compilation caching
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants