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

Adding value to attribute throws an exception #187

Closed
nabokovYaroslav opened this issue Apr 6, 2022 · 4 comments · Fixed by #205
Closed

Adding value to attribute throws an exception #187

nabokovYaroslav opened this issue Apr 6, 2022 · 4 comments · Fixed by #205
Assignees

Comments

@nabokovYaroslav
Copy link

nabokovYaroslav commented Apr 6, 2022

Adding a value to an attribute with data type "Text" throws an AttributeError referring to line 45 of eav/forms.py in the validate method with the description 'list' object has no attribute 'split'. Also, when I got into your library to figure out what was happening, I found that the error was caused by CSVFormField, although the attribute was set with the "Text" type and the value of this field in the form was empty. I also tried to enter a value when adding "1;2;3" to the CSV field and that threw another AttributeError: 'CSVFormField' object has no attribute 'separator' exception.

Expected Behavior

Value should be set

Actual Behavior

an AttributeError is thrown

Possible Fix

The to_python method returns [] because the CSV field contains an empty value, so an AttributeError exception is thrown in the validate method.
When adding a value to this field, another exception is thrown, due to the fact that the "separator" attribute was not found. The "default_separator" attribute is present, maybe you should use it?

Steps to Reproduce

  1. pip install django-eav2
  2. adding to installed apps "eav"
  3. decorator register_eav has been set to the model
  4. register the model in admin.py
  5. adding attribute with type "Text"
  6. adding value to attribute with type "Text"

Your Environment

Include relevant details about the environment you experienced the bug in.

  • Version used: django-eav2==1.2.1
  • Environment name and version: Django==3.2.12, pip 22.0.4
  • Operating System and version: image Docker python:3.8.3
  • Link to your project (if applicable): None
@Dresdn
Copy link
Contributor

Dresdn commented Apr 9, 2022

Hi @nabokovYaroslav - Thanks for reporting this! To be clear, are you talking about a datatype of Attribute.TYPE_TEXT or Attribute.TYPE_CSV? Your notes say "Test", but your write-up references CSV.

In either case, could you post a snippet to reproduce what you're seeing?

@nabokovYaroslav
Copy link
Author

Hello, @Dresdn
This error will occur in any case, regardless of the data type of the attribute. I took the "Text" data type as an example. This error occurs when adding a value (model Value) in the admin panel, referring to an error in the CSVField. I want to say that CSVField is written incorrectly.

Code from your current django-eav2 repository that is causing the error

class CSVFormField(forms.Field):
    message = _('Enter comma-separated-values. eg: one;two;three.')
    code = 'invalid'
    widget = CSVWidget
    default_separator = ";"

    def __init__(self, *args, **kwargs):
        kwargs.pop('max_length', None)
        super().__init__(*args, **kwargs)

    def to_python(self, value):
        if not value:
            return []
        return [v.strip() for v in value.split(self.separator) if v]

    def validate(self, value):
        super().validate(value)
        try:
            isinstance(value.split(self.separator), list)
        except ValidationError:
            raise ValidationError(self.message, code=self.code)

What happens when you add a value (model Value)? (It doesn't matter what type of attribute)
First case (csv field empty):

  1. The to_python method is called. The result of this method will be [] because field value is empty.
  2. The validate method is called.
isinstance(value.split(self.separator), list)

This piece of code from this method raises an AttributeError exception because value = [] and the list does not have a "split" method
Second case (csv field is not empty. For example, the value will be 1;2;3):

  1. The to_python method is called.
return [v.strip() for v in value.split(self.separator) if v]

This piece of code from this method raises an AttributeError exception because the "separator" attribute was not found.

My temporary fix

class CSVFormField(forms.Field):
    message = _('Enter comma-separated-values. eg: one;two;three.')
    code = 'invalid'
    widget = CSVWidget
    default_separator = ";"

    def __init__(self, *args, **kwargs):
        kwargs.pop('max_length', None)
        super().__init__(*args, **kwargs)

    def to_python(self, value):
        if not value:
            return []
        return [v.strip() for v in value.split(self.default_separator) if v]

    def validate(self, value):
        super().validate(value)
        try:
            isinstance(value, list)
        except ValidationError:
            raise ValidationError(self.message, code=self.code)

Sorry for writing in such detail what the problem is. I just decided to clarify the problem. I hope you understand

@Dresdn Dresdn self-assigned this May 2, 2022
@Dresdn
Copy link
Contributor

Dresdn commented May 3, 2022

@nabokovYaroslav, I spent some time and finally took a look at this. From the AdminSite, I'm only seeing issues with the CSVFormField, as other datatypes work just fine. Looking at the entire TYPE_CSV implementation, there seem to be a lot of problems, starting with your call out of CSVFormField.

  1. CSVField isn't even represented properly as value_csv() is returning a string, not a list.
  2. CSVFormField.to_python() referenced above.
  3. Calling form.is_valid() drops the value in form.cleaned_data as CSVWidget needs a value_from_datadict() override.
  4. Maybe more?

Honestly, I'm not really finding any value in the CSV Type as it'd be easy enough to store a string using a TextField, and parsing the string in your app. I'm half tempted to just implement your fix and mark the field as deprecated.

What do you think? Worth making it work properly or not?

@nabokovYaroslav
Copy link
Author

@Dresdn I don't see the point in this field either. I'd rather take it out of the library, but I can't speak for everyone. I think at the moment, it's better to make it work, that is, replace the current implementation with mine. By the way, I wanted to say that I was interested in writing my own implementation of eav without using ContentTypes, because it is obvious that it is bad in performance.

Dresdn referenced this issue in Dresdn/django-eav2 Jun 13, 2022
Dresdn added a commit that referenced this issue Jun 13, 2022
fix: #187 return csv field value in widget
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants