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

[GSoC2024] Modified Attribute Serializer to retain values after request #7736

Closed
wants to merge 2 commits into from

Conversation

mach-12
Copy link
Contributor

@mach-12 mach-12 commented Apr 6, 2024

Motivation and context

Fixing #6502: While creating or editing a multiline text attribute on a label, it was replacing the default newlines with commas.

image

This behavior was not appropriate. The cause of this issue was identified as the DelimitedStringListField serializer:

class DelimitedStringListField(serializers.ListField):
def to_representation(self, value):
return super().to_representation(value.split('\n'))
def to_internal_value(self, data):
return '\n'.join(super().to_internal_value(data))
class AttributeSerializer(serializers.ModelSerializer):
values = DelimitedStringListField(allow_empty=True,
child=serializers.CharField(allow_blank=True, max_length=200),
)
class Meta:
model = models.AttributeSpec
fields = ('id', 'name', 'mutable', 'input_type', 'default_value', 'values')

The fix was implemented by overriding the to_representation() method in the parent AttributeSerializer. This method converts the ListField into a list with only a single object with the text concatenated if the input_type in the context is 'text'.

Nested Label Serializers:

  • LabelSerializer
    • SublabelSerializer
      • AttributeSerializer
        • DelimitedStringListField

How has this been tested?

Changes persist as intended in the Tasks, Projects and Job views.

Screenshot 2024-04-07 at 12 22 01 AM

Screenshot 2024-04-07 at 12 22 19 AM

image

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@mach-12 mach-12 requested a review from Marishka17 as a code owner April 6, 2024 22:04
Copy link
Contributor

@Marishka17 Marishka17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thank you for the contribution.

I see one problem here:
In the current REST API implementation, it is possible to specify several values for text attributes too (even though it looks useless). But in that case, after these changes, all values will be merged into one and this is wrong.

We use one CharField to store values and \n to separate them. Probably you can change the separator (e.g. \\n), but in that case, Django migration must be added.

@mach-12
Copy link
Contributor Author

mach-12 commented Apr 12, 2024

it is possible to specify several values for text attributes too

Hey @Marishka17, I understand what you are trying to say here, But how does a user trigger this intended use case in the first place?

I understand you want User Input like this:

Hello,
This is a text<SEP>
Hello,
This is another text

To be stored like this:

values = ["Hello,\nThis is a text", "\nHello,\nThis is another text"]

Instead of this:

values = ["Hello,\nThis is a text\nHello,\nThis is another text"]

So I need to:

  1. Define new separator,
  2. Add appropriate db Migration

Question: How will the User know how to use it?

@Marishka17
Copy link
Contributor

But how does a user trigger this intended use case in the first place

For example, users can use REST API directly

How will the User know how to use it?

A user can check the server endpoint specification.

So I need to:
Define new separator,
Add appropriate db Migration

It is possible to fix this issue using one more way - we can restrict specifying values for text attributes since this makes no sense at all. In that case, we don't need to change the separator.

IMHO, the second approach is more preferable.

@bsekachev
Copy link
Member

Hi @mach-12

Do you have any plans regarding the pull request?

@mach-12
Copy link
Contributor Author

mach-12 commented Apr 23, 2024

Yes, I need some help with understanding how to restrict specification of text values. Going to finish this PR

@bsekachev
Copy link
Member

@Marishka17, please, work together with @mach-12 to finish the PR.

@Marishka17
Copy link
Contributor

Hello @mach-12, sorry for the delay, I'm going to close this PR now, but do not hesitate to reopen it when you implement more appropriate solution.

You can use the following approach to fix the issue:

  • on the server: add AttributeSerialize.validate method and raise ValidationError if values is not empty and input_type == models.AttributeType.TEXT
  • on the server: add migration to clean the values field for all text attributes
  • on the client: exclude sending default value in attribute values field for text attributes

@Marishka17 Marishka17 closed this May 24, 2024
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 this pull request may close these issues.

3 participants