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.

Sign up for GitHub

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

Jump to bottom

Patching the attribute set #4

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Patching the attribute set #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion geonode/layers/api/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#
#########################################################################
from rest_framework import serializers

from rest_framework.exceptions import ValidationError, ParseError
from urllib.parse import urlparse

from django.conf import settings
Expand Down Expand Up @@ -195,6 +195,37 @@ class Meta:

featureinfo_custom_template = FeatureInfoTemplateField()

def update(self, instance, validated_data):
super().update(instance, validated_data)

# Handle updates to attribute_set
allowed_fields = ["pk", "description", "attribute_label", "visible", "display_order"]
if "blob" in validated_data and "attribute_set" in validated_data["blob"]:
Copy link

@ridoo ridoo Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we rely on a blob entry here? ~This looks related to an HTML form field rendered by the DRF. ~ It turns out to be set by GeoNode itself.

However, you could handle it similar to the geoapps API It may be helpful to dive a bit deeper into how DRF validation works as well.

Update

For what I understand from the code right now, I fear you do not get any validated_data from the to_internal_value() because the attribute_set field is not writable.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what you mean, in the example you linked
_data = validated_data.pop("blob")
data is still read from the "blob" field? Do you mean it should be removed after reading like in thie example?

Copy link

Choose a reason for hiding this comment

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

@kilichenko-pixida could you debug into the flow and adjust the AttributeSerializer instead of handling updates in the DatasetSerializer instead? For what I learned when debugging the flow by myself, DRF leverages the functionality/methods provided by the DynamicRelationField itself to validate and update fields marked as writable. Place a debug point here and check what actually happens

Copy link
Author

Choose a reason for hiding this comment

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

For what I understand from the code right now, I fear you do not get any validated_data from the to_internal_value() because the attribute_set field is not writable.

That was the first thing I changed, but that wasn't a problem and even if writable the validation would fail.

Copy link
Author

Choose a reason for hiding this comment

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

@kilichenko-pixida could you debug into the flow and adjust the AttributeSerializer instead of handling updates in the DatasetSerializer instead? For what I learned when debugging the flow by myself, DRF leverages the functionality/methods provided by the DynamicRelationField itself to validate and update fields marked as writable. Place a debug point here and check what actually happens

I am not sure I am following. For example, if I add an update method to AttributeSerializer it wouldn't even be involved when doing this patch. How do you mean to adjust the AttributeSerializer?

Copy link

Choose a reason for hiding this comment

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

That was the first thing I changed, but that wasn't a problem and even if writable the validation would fail.

So, failed for what reason? You can implement validate_<field> methods (for what I could spot in the code).

For example, if I add an update method to AttributeSerializer it wouldn't even be involved when doing this patch

How do you think it is not involved? I am not an expert in this and I did have a detailed look in how this seems to work, but for what I could find out there is a field validation going on. The AttributeSerilizer in fact is known to the attribute_set field. I would suspect that the DynamicRelationField provides a mechanism for letting you implement validation. Did you have a look here already?

Copy link
Author

Choose a reason for hiding this comment

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

So, failed for what reason? You can implement validate_<field> methods (for what I could spot in the code).

How do you think it is not involved? I am not an expert in this and I did have a detailed look in how this seems to work, but for what I could find out there is a field validation going on. The AttributeSerilizer in fact is known to the attribute_set field. I would suspect that the DynamicRelationField provides a mechanism for letting you implement validation. Did you have a look here already?

The full error trace I posted here

By not involved I just mean that I tried overriding the update method of the AttributeSerilizer, but I remeber it didn't even run when doing this patch.

No, I didn't know I could reimplement validate_field methods, I'll look into the docs you just linked and see whether I can indeed write my own validation/serizliation logic for the DynamicRelationField.

Copy link

@ridoo ridoo Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Have you tried looking at https://www.django-rest-framework.org/api-guide/serializers/#writing-update-methods-for-nested-representations ?

I mean, it is not wrong to place things in the DatasetSerializer. For what I am concerned about is the blob or data parts. Just send the (unwrapped) attribute_set part.

Copy link
Author

Choose a reason for hiding this comment

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

I think I skimmed through this one, I thought DRF docs were more relevant, I will have a deeper look

Copy link
Author

@kilichenko-pixida kilichenko-pixida Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I mean, it is not wrong to place things in the DatasetSerializer. For what I am concerned about is the blob or data parts. Just send the (unwrapped) attribute_set part.

Yeah, I realize it would be preferrable to not have it wrapped and I don't have any strong opinions on where it should be handled if you think there is a better place for it than DatasetSerializer.

Just sending attribute_set unwrapped would fail the default validation/convertion to internal values and not even be passed to the update method. Maybe it's as obvious and as simple as overriding some method to avoid default validation but I just lack the background - I'd appreciate a hint if you have one, otherwise I'd dig into documentation after I'm back and try to do it.

attributes = validated_data["blob"]["attribute_set"]

for attribute in attributes:
for field, _ in attribute.items():
if field not in allowed_fields:
raise ValidationError(
f"{field} is not one of the fields that could be edited directly. \
Only {str(allowed_fields)} are allowed"
)

for attribute in attributes:
try:
if "pk" in attribute:
attribute_instance = Attribute.objects.get(pk=attribute["pk"], dataset=instance)
Copy link

@ridoo ridoo Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please handle the geonode.layers.models.Attribute.DoesNotExist case .. i.e. when pk is unknown

Copy link
Author

@kilichenko-pixida kilichenko-pixida Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll take care of the case with the pk unknown.

Regarding the "data" field - in our meeting I was patching through the geonodectl, so it's taken care of there, but yes, it's still necessary do wrap it when endpoint is used directly.

I spent first several days on the ticket trying to make "'{ "attribute_set":[{ "pk": 1, "description": "updated description" }]}'" payload work or understand why exactly is doesn't, but I still don't know, serializer expects this field to be a list of integers and would just throw exceptions if given anything else. Didn't find documentation for DRF on that specifically and there is a ton of serializer code involved.

Copy link
Author

Choose a reason for hiding this comment

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

Please handle the geonode.layers.models.Attribute.DoesNotExist case .. i.e. when pk is unknown

Do you mean just adding a more meaningful message to it?

for field, value in attribute.items():
setattr(attribute_instance, field, value)
attribute_instance.save()
else:
raise Exception("Primary key of the attribute to be patched not specified")
except Exception as e:
logger.error(e)
raise ParseError(str(e))

return instance


class DatasetListSerializer(DatasetSerializer):
class Meta(DatasetSerializer.Meta):
Expand Down