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

232 post entity will not completely override the existing entity #233

Conversation

djs0109
Copy link
Contributor

@djs0109 djs0109 commented Jan 17, 2024

closes #232

@djs0109
Copy link
Contributor Author

djs0109 commented Jan 17, 2024

@tstorek by checking this issue, I found that the patch_entity() function is very heavy. And its behavior is more like a PUT request. Do you have an idea about that?

@djs0109 djs0109 requested a review from sbanoeon January 17, 2024 15:06
@tstorek
Copy link
Collaborator

tstorek commented Jan 18, 2024

@djs0109 The reason for patch_entity is that Orion does not support partial updates. (Maybe using complex query). Hence, we introduced this one to allow for the functionality. Hence, it should merge both contents. And only overwriting those that should be updated or appended.

@djs0109
Copy link
Contributor Author

djs0109 commented Jan 18, 2024

@tstorek By "partially update," do you mean like to send only a part of the attributes in the payload, and then Orion will only update the included attributes (and other attributes will not be changed)? If not, it would be great if you can add a simple implementation here

def test_entity_update(self):

@djs0109 djs0109 requested review from RCX112 and removed request for sbanoeon February 6, 2024 15:12
Copy link
Collaborator

@RCX112 RCX112 left a comment

Choose a reason for hiding this comment

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

There are only some small comments from my side :)

client.post_entity(entity=entity_post, update=True)
self.assertEqual(client.get_entity(entity_id=entity_post.id),
entity_post)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@djs0109 After each step (so after post, update_entity and override_entity) I would clear the OCB and reset the variable attr_append:

clear_all(fiware_header=self.fiware_header, cb_url=settings.CB_URL)
attr_append = NamedContextAttribute(**{
    "name": 'pressure',
    "type": 'Number',
    "value": 1050})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I add the clear_all, and use attr_append_update for updating attribute, so that the attr_append will not be touched during the whole test.

# 1) append attribute
entity_update.add_attributes(attrs=[attr_append])
with self.assertRaises(requests.RequestException):
client.update_entity(entity=entity_update, append_strict=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@djs0109 This call still makes an update to the entity. So we could add the follwing assertion:

self.assertEqual(client.get_entity(entity_id=entity_update.id),
                                 entity_update)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I thought that the request will be completely blocked

# 3) delete attribute
entity_patch.delete_attributes(attrs=[attr_append])
client.patch_entity(entity=entity_patch)
# TODO It is expected here not to pass the test
Copy link
Collaborator

Choose a reason for hiding this comment

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

@djs0109 The patch function can also delete attributes from an entity with the DELETE request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Currently it is possible. But right now patch_entity works more like a put_entity. But I will remove the todos here and maybe open another issue

@djs0109
Copy link
Contributor Author

djs0109 commented Feb 14, 2024

@tstorek It seems that the patch end points are supported now in orion v2 check the section PATCH /v2/entities/{entityId}/attrs in https://fiware-orion.readthedocs.io/en/master/orion-api.html . If I understand correctly, the real functionality of patch is implemented in update_existing_entity_attributes. Do you think it is a good idea to rename the methods to avoid missunderstanding? But the decision should be irrelevant for this issue

@RCX112
Copy link
Collaborator

RCX112 commented Feb 20, 2024

@djs0109 I have only one small comment on the line

client.get_entity(entity_id=self.entity.id, attrs=['temperature'])

because it does not do very much. You can maybe make an assertion out of it like
self.assertEqual(res_entity, client.get_entity(entity_id=self.entity.id, attrs=['temperature'])). I think we can merge after this.

@djs0109
Copy link
Contributor Author

djs0109 commented Feb 20, 2024

@RCX112 Thank you for the suggestion! But I think that will make the test a bit more hard-coded, because if we later add another initial attribute (e.g. "pressure") in self.entity, the code you suggested will not work. What do you think? If you think this point is critical, I can still change it.

@RCX112
Copy link
Collaborator

RCX112 commented Feb 20, 2024

@djs0109 Maybe you could change the line to something like this pseudo code:
self.assertEqual(res_entity, client.get_entity(entity_id=self.entity.id, attrs=get_all_attributes_of_self.entity)). This should be more flexible. If we keep the line as it is, this code only test the excecution without looking at the result.

@djs0109
Copy link
Contributor Author

djs0109 commented Feb 20, 2024

@RCX112 Fair point :) I added it like that

Copy link
Collaborator

@RCX112 RCX112 left a comment

Choose a reason for hiding this comment

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

@djs0109 I think the branch is ready to merge :)

@djs0109 djs0109 merged commit 4ba3441 into master Feb 20, 2024
1 check passed
@djs0109 djs0109 deleted the 232--post_entity-will-not-completely-override-the-existing-entity branch February 20, 2024 16:34
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.

post_entity will not completely override the existing entity
3 participants