-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support dots in claim names #95
Support dots in claim names #95
Conversation
Also updated the existing tests for the new expected data structure.
The glom properties now expect a list of strings as path bits, rather than a single string with periods signifying a level of nesting. Old: foo.bar New: [foo, bar]
The field abstracts away the underlying ArrayField usage. A data migration is included that copies the existing configuration into the new format.
Drop the old fields and rename the new ones to the old field names
Tests no longer reflected the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, tested it locally and it works good too 👍, just the question about the SOLO_CACHE
.
Can you also update the documentation about claims and dotted paths or create a separate issue for this?
mozilla_django_oidc_db/migrations/0002_migrate_to_claim_field.py
Outdated
Show resolved
Hide resolved
|
||
|
||
def flush_cache(): | ||
if not (cache_name := getattr(settings, "SOLO_CACHE", "")): | ||
cache_name = getattr( | ||
settings, "OIDC_CACHE", oidc_settings.MOZILLA_DJANGO_OIDC_DB_CACHE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should try to get MOZILLA_DJANGO_OIDC_DB_CACHE
from django.conf.settings
first and fall back on the oidc_settings
if it's not defined, otherwise it could differ from the configured MOZILLA_DJANGO_OIDC_DB_CACHE
in a project that overrides it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh, is CachingMixin.clear_cache
wrong then? Since that's where I grabbed the one-liner from.
I can see that set_to_cache
indeed uses this setting, but clear_cache
uses another one 🤔
django-solo uses the same settings names for clear_cache
, set_to_cache
and get_solo
methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm you're right 🤔. I think that clear_cache
should indeed use MOZILLA_DJANGO_OIDC_DB_CACHE
, since this is the setting that is used in the library (and also the one that is set in OF itself). If you want you can change this in this PR or create an issue for it
8527b31
to
d25ea89
Compare
Closes #94
Relates to open-formulieren/open-forms#4246
I squashed the existing migrations to clean up a bit too.
Downstream projects that add their own custom properties will need to manage their migrations themselves, but the custom field can be used to get a consistent experience.