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

Tags longer than 100 chars raise 500 error #872

Open
halitcelik opened this issue Oct 16, 2023 · 15 comments
Open

Tags longer than 100 chars raise 500 error #872

halitcelik opened this issue Oct 16, 2023 · 15 comments
Labels

Comments

@halitcelik
Copy link

In wagtail admin where django-taggit is being used for documents this error happens. It is mentioned in this wagtail issue. Locally I could fix the issue changing these lines to:

def clean(self, value):
        value = super().clean(value)
        try:
            for tag in value.split(","):
                if len(tag) > 100:
                    raise forms.ValidationError(
                _("One or more tag(s) exceed max length (100 chars).")
            )
        except ValueError:
            forms.ValidationError(
                _("Please provide a comma-separated list of tags.")
            )
        try:
            return parse_tags(value)
        except ValueError:
            raise forms.ValidationError(
                _("Please provide a comma-separated list of tags.")
            )

I am not sure if this is the correct way to fix it. Please let me know if it is I can create a PR.

@Temidayo32
Copy link

@halitcelik are you also getting this error working off your development server?

@halitcelik
Copy link
Author

@Temidayo32 I saw the discussions in wagtail issues. So, maybe we can discuss possible solutions. I'm happy to help if I can.

@Temidayo32
Copy link

@halitcelik I would be interested in that

@halitcelik
Copy link
Author

@Temidayo32 I'm already halfway through for a PR. It would be good to get the opinion of the maintainer.

@Temidayo32
Copy link

@halitcelik Great!

@Temidayo32
Copy link

@halitcelik I made a pull request that resolves this in Wagtail. Hopefully, it get merged!

wagtail/wagtail#11119

@rtpg
Copy link
Contributor

rtpg commented Oct 24, 2023

@halitcelik this is a bit tricky, because the max length in TagBase of 100 is not really accessible way over in TagField.

What I think would be good is the following:

  • Change TagField to take an optional parameter in the constructor, max_tag_length, defaulting to 100
  • Use that in the validator like in the snippet you have (perhaps making it so that max_tag_length of None opts out of the validation)
  • Noting that there's an extra validator on TagField in the changelog (and setting to None is good)

This would make the field work "as expected" out of the box, and offer a straightforward way forward for people upgrading in existing setups.

Tell me if you're up for the PR! If you have a WIP set of changes, just pushing that up is also fine (I can make any final changes to get it to work if you want)

@rtpg rtpg added the bug label Oct 24, 2023
@halitcelik
Copy link
Author

@rtpg Thanks a lot. I'll create a PR. I thought this would not break the existing setups because we already have a database validation for 100 max length.

@halitcelik
Copy link
Author

@rtpg I created a PR: #874
I am a bit confused about passing None in the Tagfield max_tag_length. If somebody sets it to None and we skip validation, it will still raise 500 in a later step. What do you think?

@BertrandBordage
Copy link

@rtpg You mentioned:

this is a bit tricky, because the max length in TagBase of 100 is not really accessible way over in TagField.

What do you mean, it's not accessible? Can't we do this?
image

Then no need to worry about None. The max_length should always be set, unless in the future we have some way of defining TagBase.name as a TextField using a swappable model 🤔

@rtpg
Copy link
Contributor

rtpg commented Oct 25, 2023

@BertrandBordage well, you can customize the through model in TaggableManager and just show up with another tag model entirely (that just follows a similar API, but might back the name with a text field or something else).

This is one of those things where I wouldn't be surprised if nobody is actually in this scenario, but I would also not be surprised if someone was in this scenario, so I like a PR that handles both cases. Having said that.... I dunno, maybe that's a bit too wild of a concept

@BertrandBordage
Copy link

@rtpg I see, if you override name in a custom TagBase descendant and rewrite tagged items to use it. It's a shame, I thought only tagged items were generally customized 😕

@BertrandBordage
Copy link

TaggableManager has a method for generating a form field, and it has access to the relation, meaning it can introspect the tag model. Can't we use that, combined with a new max_tag_length constructor parameter for TagField?
Of course, we would pass max_tag_length only if issubclass(form_class, TagField).

And in the doc, we would mention the need to fill max_tag_length when TagField is used manually, outside of the context of TaggableManager.

@hamidrabedi
Copy link

Same issue here with drf:

class BlogSerializer(TaggitSerializer, serializers.ModelSerializer):
    tags = TagListSerializerField()
    class Meta:
        model = Blog
        fields = (
            "tags",
        )

and when i try to add one tag with more that 100 character:

django.db.utils.DataError: value too long for type character varying(100)

@29deepanshutyagi
Copy link

i want to solve this issue , if this issue is still open @Temidayo32 and if possible kindly gave me required information to solve this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants