Convert longer media varchar fields to text in the API
#4315
+183
−37
There are no files selected for viewing
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| # Generated by Django 4.2.11 on 2024-05-10 22:07 | ||
|
|
||
| import api.models.fields | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('api', '0060_fill_out_help_text'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='audio_set_foreign_identifier', | ||
| field=models.TextField(blank=True, help_text='Reference to set of which this track is a part.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='creator', | ||
| field=models.TextField(blank=True, help_text='The name of the media creator.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='creator_url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='A direct link to the media creator.', max_length=2000, null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='foreign_identifier', | ||
| field=models.TextField(blank=True, db_index=True, help_text='The identifier provided by the upstream source.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='foreign_landing_url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The landing page of the work.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='thumbnail', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The thumbnail for the media.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='title', | ||
| field=models.TextField(blank=True, help_text='The name of the media.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audio', | ||
| name='url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The actual URL to the media file.', max_length=1000, null=True, unique=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audioset', | ||
| name='creator', | ||
| field=models.TextField(blank=True, help_text='The name of the media creator.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audioset', | ||
| name='creator_url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='A direct link to the media creator.', max_length=2000, null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audioset', | ||
| name='foreign_identifier', | ||
| field=models.TextField(blank=True, db_index=True, help_text='The identifier provided by the upstream source.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audioset', | ||
| name='foreign_landing_url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The landing page of the work.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audioset', | ||
| name='thumbnail', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The thumbnail for the media.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audioset', | ||
| name='title', | ||
| field=models.TextField(blank=True, help_text='The name of the media.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='audioset', | ||
| name='url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The actual URL to the media file.', max_length=1000, null=True, unique=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='image', | ||
| name='creator', | ||
| field=models.TextField(blank=True, help_text='The name of the media creator.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='image', | ||
| name='creator_url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='A direct link to the media creator.', max_length=2000, null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='image', | ||
| name='foreign_identifier', | ||
| field=models.TextField(blank=True, db_index=True, help_text='The identifier provided by the upstream source.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='image', | ||
| name='foreign_landing_url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The landing page of the work.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='image', | ||
| name='thumbnail', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The thumbnail for the media.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='image', | ||
| name='title', | ||
| field=models.TextField(blank=True, help_text='The name of the media.', null=True), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='image', | ||
| name='url', | ||
| field=api.models.fields.URLTextField(blank=True, help_text='The actual URL to the media file.', max_length=1000, null=True, unique=True), | ||
| ), | ||
| ] |
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| from django import forms | ||
| from django.core import validators | ||
| from django.db import models | ||
| from django.utils.translation import gettext_lazy as _ | ||
|
|
||
|
|
||
| class URLTextField(models.TextField): | ||
| """URL field which uses the underlying Postgres TEXT column type.""" | ||
|
|
||
| default_validators = [validators.URLValidator()] | ||
| description = _("URL") | ||
|
|
||
| def formfield(self, **kwargs): | ||
| # As with CharField, this will cause URL validation to be performed | ||
| # twice. | ||
|
Comment on lines
+14
to
+15
There was a problem hiding this comment. Twice meaning it would happen in the form and at the database level? |
||
| return super().formfield( | ||
| **{ | ||
| "form_class": forms.URLField, | ||
| **kwargs, | ||
| } | ||
| ) | ||
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| # This file is autogenerated by makemigrations. | ||
| # If you have a merge conflict in this file, it means you need to run: | ||
| # manage.py makemigrations --merge | ||
| # in order to resolve the conflict between migrations. | ||
|
|
Oops, something went wrong.
Oops, something went wrong.
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.
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.
(Up front context) I know we currently use a URL field for some of these (though this would be a new detail for
creator_url, I think), so this is only relevant feedback for this PR due to the need to add new code to continue supporting it. Just wanting to clarify up front I know this isn't a decision you're making about the model, and that my intention is to ask whether we need this at all, and if not, then to give feedback that this code is unnecessary and can be removed.This validator is the only thing that differentiates the URL field from a regular text field. I question whether we need this. Not blocking because it's a trivial thing to remove in the future or as a fast-follow if we like (would be a no-SQL migration). I'd prefer we didn't add this code, however. These write-time Django validators are irrelevant for our domain and usage of the ORM because we never write to these tables with the Django ORM (except in tests). Data validation either has to happen in the catalogue, data refresh, or not at all, but definitely not in Django write-time validators.
The change requested would be to use a
TextFieldand forego write-time validators as a code-quality improvement and clarification of the intention of these models and the domain they're actually concerned with (i.e. reading).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.
That's an interesting point. I can't think of a use case right now, but I also think it doesn't hurt to leave the validation for this kind of field. I have no strong opinion on either side.
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 am very happy to remove this code as a fast-follow! I wanted to make as functionally minimal a change as possible here, even if it meant adding more code. I'll follow up with an issue and a PR later this week.
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.
@AetherUnbound Just checking, were you able to create the issue?
@krysal I guess generally there is a rationale of removing code that isn't used. All code is a liability, either as a vulnerability or increased maintenance cost (the latter being most relevant and in fact exemplified here), so as much of it as we don't need is a good idea to remove.
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.
Created in #4320