DRAFT django modernization#1564
Draft
annamontare-nava wants to merge 34 commits into
Draft
Conversation
Moved `phone_regex` validator up to the apps level and use it for in both `accounts/models.py` and `dot_ext/models.py`.
This is to align to the spec. Also, add a TODO comment about something confusing
This should make all the models consistently using TextField + URLValidator for URLs
This matches all the other URLs
Best practice is to commit migrations in the same commit as the changes to the models, so I will try to do this going forward.
This is the default for URLValidator, but we set it explicitly on the TextField to make it clear.
This does suggest that it might be easy for the different fields to become out of sync.
Contributor
Author
|
Noticed that a lot of fields are named "uri" when it might be more specific to say "url". I assume this would not be worth changing? |
Change the max_length to match the associated field, and use the same validator as in the model.
Will be mentioned to reviewers
Contributor
|
@annamontare-nava what is the latest here? Will this branch be picked up again in the future? |
Contributor
Author
I think Jimmy suggested I prioritize other work, so haven't worked on it in a bit, but I'd like to pick it up in the future and finish the work. |
Contributor
There was a problem hiding this comment.
Pull request overview
This draft PR modernizes several Django models and related validation by standardizing field types/lengths and moving toward built-in Django validators (notably for URLs and phone numbers), with accompanying schema migrations.
Changes:
- Introduces a shared
phone_regexvalidator and applies it to application/support and user profile phone fields. - Updates multiple model URL fields to
TextFieldwithURLValidatorand increases max_length limits (e.g., to 2048), removing a custom URL-regex validator implementation. - Refactors
AuthFlowUuid/AuthFlowUuidCopyto share an abstract base model and adjusts several max_length constraints to align with referenced specs (e.g., FHIR id length).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/validators.py | Adds shared phone_regex validator for reuse across apps. |
| apps/pkce/models.py | Tightens PKCE field lengths to match RFC guidance. |
| apps/pkce/migrations/0003_alter_codechallenge_challenge_and_more.py | Applies PKCE max_length/choices changes at the DB layer. |
| apps/mymedicare_cb/models.py | Adds URL validation metadata to AnonUserState.next_uri and changes state to TextField. |
| apps/mymedicare_cb/migrations/0004_alter_anonuserstate_next_uri_and_more.py | Alters AnonUserState fields (including adding URLValidator + length). |
| apps/mymedicare_cb/migrations/0005_alter_anonuserstate_next_uri.py | Follow-up migration further increasing next_uri length. |
| apps/fhir/bluebutton/models.py | Reduces FHIR id max_length to 64 and updates archived crosswalk documentation/fields. |
| apps/fhir/bluebutton/migrations/0012_alter_archivedcrosswalk_fhir_id_v2_and_more.py | Applies FHIR id length changes to Crosswalk/ArchivedCrosswalk. |
| apps/dot_ext/validators.py | Removes custom validate_url function and related constant usage. |
| apps/dot_ext/tests/test_validators.py | Removes tests that previously asserted URL validation behavior. |
| apps/dot_ext/models.py | Broad model updates: URL fields -> TextField + URLValidator, shared phone validator, and AuthFlowUuid refactor. |
| apps/dot_ext/migrations/0012_alter_application_client_uri_and_more.py | Alters many dot_ext application/auth fields; introduces some URL/phone changes. |
| apps/dot_ext/migrations/0013_alter_application_client_uri_and_more.py | Immediately re-alters several of the same URL fields (mostly max_length changes). |
| apps/dot_ext/migrations/0014_alter_application_op_policy_uri_and_more.py | Further URL-field alterations. |
| apps/dot_ext/migrations/0015_alter_application_client_secret_plain.py | Adjusts client_secret_plain length. |
| apps/dot_ext/migrations/0016_alter_authflowuuid_client_id.py | Adjusts AuthFlowUuid client_id length. |
| apps/dot_ext/migrations/0017_alter_authflowuuid_auth_pkce_method_and_more.py | Adds PKCE method choices + changes AuthFlowUuidCopy created semantics. |
| apps/dot_ext/forms.py | Switches URL form validation from custom validator to Django URLValidator. |
| apps/dot_ext/constants.py | Removes custom URL_REGEX constant and associated imports. |
| apps/accounts/models.py | Applies shared phone validator and expands phone field length. |
| apps/accounts/migrations/0006_alter_userprofile_mobile_phone_number.py | Applies phone validator/length changes at the DB layer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| regex=r'^\+?1?\d{9,15}$', | ||
| message="Phone number must be entered in the format: '+999999999'. Up to 15 digits allowed.", | ||
| ) | ||
| support_email = models.TextField(blank=True, null=True) |
| # https://datatracker.ietf.org/doc/html/rfc7636#section-4.3 | ||
| auth_pkce_method = models.CharField(max_length=16, null=True, choices={'S256': 'S256', 'plain': 'plain'}) | ||
|
|
||
| created = models.DateTimeField(auto_now_add=True, null=True) |
Comment on lines
79
to
+82
| # where the client_secret is hashed ireversible | ||
| client_secret_plain = models.CharField(default='', blank=True, max_length=255) | ||
| # | ||
| # In ouath2_provider, the default length of the generated secret is 128, | ||
| # but the max_length of AbstractApplication.client_secret is 255. Going with |
Comment on lines
+542
to
+543
| # matches what is in django rest framework | ||
| # https://github.com/django-oauth/django-oauth-toolkit/blob/d422eeab79052c04a91d124f938ddc22c841c38c/oauth2_provider/models.py#L333 |
Comment on lines
103
to
107
| mobile_phone_number = models.CharField( | ||
| max_length=12, | ||
| validators=[phone_regex], | ||
| max_length=16, | ||
| blank=True, | ||
| help_text=_('US numbers only.'), |
Comment on lines
+93
to
96
| # https://www.hl7.org/fhir/R4/datatypes.html#id | ||
| fhir_id_v2 = models.CharField( | ||
| max_length=80, | ||
| max_length=64, | ||
| null=True, |
Comment on lines
52
to
57
| policy_uri = forms.CharField( | ||
| required=False, | ||
| max_length=512, | ||
| validators=[validate_url], | ||
| max_length=2048, | ||
| validators=[URLValidator()], | ||
| widget=URLInput, | ||
| ) |
Comment on lines
+9
to
+18
| dependencies = [ | ||
| ('mymedicare_cb', '0004_alter_anonuserstate_next_uri_and_more'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( | ||
| model_name='anonuserstate', | ||
| name='next_uri', | ||
| field=models.TextField(default='', max_length=2048, validators=[django.core.validators.URLValidator()]), | ||
| ), |
Comment on lines
+14
to
+18
| migrations.AlterField( | ||
| model_name='application', | ||
| name='client_uri', | ||
| field=models.TextField(blank=True, default='', help_text='This is typically a home/download website for the application. For example, https://www.example.org or http://www.example.org .', max_length=2048, null=True, validators=[django.core.validators.URLValidator()], verbose_name='Client URI'), | ||
| ), |
Comment on lines
+444
to
+445
| state = models.TextField(default='', max_length=64, db_index=True) | ||
| next_uri = models.TextField(default='', validators=[URLValidator()], max_length=2048) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What needs to happen to get this ready for review:
JIRA Ticket:
epic: BB2-4543
What Does This PR Do?
Changes to Django models to migrate
CharField's toTextField's, makemax_lengthsettings consistent with specs, and align validators across fields.Also, fix a doc comment and remove duplicate code.
I did not include Ruff changes to hopefully make this easier to review.
What Should Reviewers Watch For?
If you're reviewing this PR, please check for these things in particular:
max_lengthseemed to match what oauth2_provider had formax_length, but not the actual length of the values. I changed them to match the actual length, and left a note, but would we prefer having them match what's in oauth2_provider?statefields still have amax_length?For fields that are URLs, I chose to make all of them
TextField's withURLValidator's and an explicitmax_lengthof 2048. Counterproposals:- Make all URLs
URLField's. Pros: uses the tools Django provides, arguably what a new person to the project might expect the fields to be. Cons: Is aCharFieldsubclass; there have been issues with the defaultmax_lengthin the past, so we'd have to set a larger one.- Make a subclass of
TextFieldthat includes the validator and our chosenmax_length, and make all URLs use that. Pros: It's aTextField, codifies the consistent handling of URLs across models. Cons: might still be easy to forget in new code.Validation
What Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
security engineer's approval.
Any Migrations?
etc)