Patching the attribute set #4

kilichenko-pixida · 2023-11-30T00:52:23Z

Resolves https://github.com/Thuenen-GeoNode-Development/Sprints/issues/14 -- patching the attribute set of the dataset

Overriding update method in dataset serializer class s.t. it can patch the attribute set of the datasets. In order to patcth the attribute set it should be passed in the following format to the PATCH /api/v2/datasets/2 endpoint:
{"data": {"attribute_set": [{...}, ...]}}, see the
See related PR for the geonodectl

Also related GNIP

ridoo · 2023-11-30T09:52:30Z

in the following format to the PATCH /api/v2/datasets/2 endpoint:
{"data": {"attribute_set": [{...}, ...]}},

@kilichenko-pixida is this actually true still? In our meeting you patched somethings like

{
  "attribute_set": [
    {
      "id": "23",
      "description": "some description"
    }
  ]
}

so without the "data" part.

Edit: See my comment/review below

ridoo

@kilichenko-pixida I still need to add a wrapping "data" attribute to make PATCHing the attribute_set work:

curl -X PATCH -Lk http://172.17.0.1:8001/api/v2/datasets/1 \
  -H "Content-Type: application/json" \
  -H "Authorization: Basic $(echo -n admin:admin | base64 )" \
  -d '{"data": { "attribute_set":[{ "pk": 1, "description": "updated description" }]}}'

I expected that this works instead of the above:

curl -X PATCH -Lk http://172.17.0.1:8001/api/v2/datasets/1 \
  -H "Content-Type: application/json" \
  -H "Authorization: Basic $(echo -n admin:admin | base64 )" \
  -d '{ "attribute_set":[{ "pk": 1, "description": "updated description" }]}'

ridoo · 2023-11-30T10:52:59Z

geonode/layers/api/serializers.py

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


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

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.

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?

ridoo · 2023-11-30T11:09:23Z

geonode/layers/api/serializers.py

+
+        # 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"]:


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.

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?

@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

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.

@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?

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?

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.

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.

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

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.

ridoo · 2023-11-30T12:43:56Z

geonode/layers/api/serializers.py

+
+        # 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"]:


@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

ridoo

As discussed in the Sprint meeting, we will move this discussion to upstream to gain feedback from GeoSolutions. This includes merging the status into thuenen_atlas. In the upstream issue/GNIP @kilichenko-pixida will summarize the current discussion and create a PR which should our current solution and why we are unhappy with it at the moment.

ridoo · 2023-12-04T13:52:40Z

@kilichenko-pixida please add a link to the upstream discussion here as well

kilichenko-pixida · 2023-12-12T19:31:15Z

The relevant upstream issue and discussion could be found here.

overriding update method in dataset serializer class

628e5f1

kilichenko-pixida requested a review from ridoo November 30, 2023 00:52

kilichenko-pixida self-assigned this Nov 30, 2023

This was referenced Nov 30, 2023

A new parameter to read json files to patch dataset metadata (including attributes) GeoNodeUserGroup-DE/geonodectl#30

Merged

allow patch json from file GeoNodeUserGroup-DE/geonodectl#11

Closed

ridoo requested changes Nov 30, 2023

View reviewed changes

ridoo reviewed Nov 30, 2023

View reviewed changes

ridoo requested changes Nov 30, 2023

View reviewed changes

ridoo approved these changes Dec 4, 2023

View reviewed changes

ridoo merged commit 7ed45ed into thuenen_4.x Dec 4, 2023
6 checks passed

ridoo mentioned this pull request Dec 4, 2023

Feature Branch Status Thuenen-GeoNode-Development/thuenen_atlas#8

Open

30 tasks

kilichenko-pixida mentioned this pull request Dec 12, 2023

GNIP 93: Allowing changes to the dataset attributes table via REST API GeoNode/geonode#10873

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patching the attribute set #4

Patching the attribute set #4

kilichenko-pixida commented Nov 30, 2023 •

edited by ridoo

Loading

ridoo commented Nov 30, 2023 •

edited

Loading

ridoo left a comment

ridoo Nov 30, 2023 •

edited

Loading

kilichenko-pixida Nov 30, 2023 •

edited

Loading

kilichenko-pixida Nov 30, 2023

ridoo Nov 30, 2023 •

edited

Loading

kilichenko-pixida Nov 30, 2023

ridoo Nov 30, 2023

kilichenko-pixida Nov 30, 2023

kilichenko-pixida Nov 30, 2023

ridoo Nov 30, 2023

kilichenko-pixida Nov 30, 2023

ridoo Dec 1, 2023 •

edited

Loading

kilichenko-pixida Dec 1, 2023

kilichenko-pixida Dec 1, 2023 •

edited

Loading

ridoo Nov 30, 2023

ridoo left a comment •

edited

Loading

ridoo commented Dec 4, 2023

kilichenko-pixida commented Dec 12, 2023

Patching the attribute set #4

Patching the attribute set #4

Conversation

kilichenko-pixida commented Nov 30, 2023 • edited by ridoo Loading

ridoo commented Nov 30, 2023 • edited Loading

ridoo left a comment

Choose a reason for hiding this comment

ridoo Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

kilichenko-pixida Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridoo Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridoo Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kilichenko-pixida Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridoo left a comment • edited Loading

Choose a reason for hiding this comment

ridoo commented Dec 4, 2023

kilichenko-pixida commented Dec 12, 2023

kilichenko-pixida commented Nov 30, 2023 •

edited by ridoo

Loading

ridoo commented Nov 30, 2023 •

edited

Loading

ridoo Nov 30, 2023 •

edited

Loading

kilichenko-pixida Nov 30, 2023 •

edited

Loading

ridoo Nov 30, 2023 •

edited

Loading

ridoo Dec 1, 2023 •

edited

Loading

kilichenko-pixida Dec 1, 2023 •

edited

Loading

ridoo left a comment •

edited

Loading