Skip to content
This repository has been archived by the owner on Jun 13, 2023. It is now read-only.

fix(pymongo): extend support for more operations #394

Merged
merged 9 commits into from
Jan 17, 2022

Conversation

sagivr2020
Copy link
Member

@sagivr2020 sagivr2020 requested a review from a team as a code owner January 2, 2022 11:17
@sagivr2020 sagivr2020 requested a review from maorlx January 2, 2022 13:43
Copy link
Member

@maorlx maorlx left a comment

Choose a reason for hiding this comment

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

great PR ! :)
See my comments, specifically regarding the request payload collection (document/documents)

@@ -19,6 +19,8 @@ class PyMongoEvent(BaseEvent):

ORIGIN = 'pymongo'
RESOURCE_TYPE = 'pymongo'
INSERT_OPERATIONS = ['insert_one', 'insert_many']
Copy link
Member

Choose a reason for hiding this comment

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

great putting that in a const.
Would change it to be a tuple instead of a list

[str(response.inserted_id)]

if self.resource['operation'] in PyMongoEvent.INSERT_OPERATIONS:
for i in range(len(self.resource['metadata'].get('items', []))):
Copy link
Member

Choose a reason for hiding this comment

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

I would move each logic (of insert/query operations) to a separated method to handle the response data

add_data_if_needed(self.resource['metadata'], 'Items', documents)

elif self.resource['operation'] in PyMongoEvent.FILTER_OPERATIONS:
add_data_if_needed(self.resource['metadata'], 'Search', documents)
Copy link
Member

Choose a reason for hiding this comment

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

not sure regarding the "Search" key, consider renaming it (we can talk about it)

}

if self.resource['operation'] in PyMongoEvent.INSERT_OPERATIONS:
add_data_if_needed(self.resource['metadata'], 'Items', documents)
Copy link
Member

Choose a reason for hiding this comment

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

I think there's an issue with the document/documents logic (regarding the insert one/many operations)
Also, the documents param is only relevant for specific operations. So, I think I would add some payload colelction here according to the operation (insert many > so it documents, insert one/update one > document, etc)

@epsagon epsagon deleted a comment from maorlx Jan 4, 2022
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 4, 2022
@sagivr2020 sagivr2020 requested a review from maorlx January 4, 2022 13:28
self.resource['metadata']['Results'] = \
list(response)

elif self.resource['operation'] in ['update_one']:
Copy link
Member

Choose a reason for hiding this comment

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

better check if its equal to update_one (if we will have one more similar operation, we can add the "in" check)

self.resource['metadata']['modified_count'] = \
response.modified_count

elif self.resource['operation'] in ['delete_many']:
Copy link
Member

Choose a reason for hiding this comment

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

same as update_one comment

:return: None
"""

self.resource['metadata']['Filter'] = str(
Copy link
Member

Choose a reason for hiding this comment

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

should put it directly as str in the handle_request_payload function

'Collection Name': str(instance.collection.name),
}

self.handle_request_payload(input_args)
Copy link
Member

Choose a reason for hiding this comment

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

perfect :)

input_args = list()

else:
input_args = args[0]
Copy link
Member

Choose a reason for hiding this comment

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

I think the [0] logic should be done inside the handle_request_payload (according to the operation , you can know what to expect in args)
So basically, you would just send "args" as is to handle_request_payload. The handle_request_payload obviously should be called only if there're any args

INSERT_ONE = 'insert_one'
INSERT_MANY = 'insert_many'
INSERT_OPERATIONS = (INSERT_ONE, INSERT_MANY)
FILTER_OPERATIONS = ['find', 'update_one', 'delete_many']
Copy link
Member

Choose a reason for hiding this comment

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

change to tuple

Copy link
Member

@maorlx maorlx left a comment

Choose a reason for hiding this comment

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

Great :)

@sagivr2020 sagivr2020 merged commit 937bd16 into master Jan 17, 2022
@ranrib
Copy link
Member

ranrib commented Jan 17, 2022

🎉 This PR is included in version 1.77.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants