Feat/Context Managers for Document and Security Document #35

adeelsohailahmed · 2021-07-24T00:39:49Z

Adds asynchronous context managers to both Document and Security Document.

These context managers automatically:

Retrieve the document from the remote server and overwrite local data, if document already exists
Upon exit, update the remote document with the updated local data, creating a new document if it didn't exist before.
Don't create/update the document if there were uncaught exceptions in the with block.

I've also added the appropriate tests.

An example:

async with CouchDB(server=SERVER_URL, user=USER, password=PASSWORD) as couchdb:
    db = await couchdb.create("test_db", exists_ok=True)
    async with Document(database=db, id="new_doc") as doc:
        doc["king"] = "elvis"

Inspired from (now deprecated) python-cloudant. Their examples for reference: Document Context Manager & Security Document Context Manager

PS: Thank you for this amazing library.

Add async context manager to Security Document;

(one each for Document & Security Document)

bmario · 2021-07-29T17:14:35Z

Thank you for your PR. Now that you brought it up, the missing context managers look like a major oversight on my side.

However, I'm having two concerns regarding your implementation. For aiocouch, I see using the constructors of the different classes more as an advanced feature. I want users to use the member functions. For example, use Database.__getitem__ instead of Document.__init__. That is certainly a different approach than python-cloudant. But context managers make so much sense, so I'd like to see them as return values for member functions as well.

The other thing I'm not convinced of is silently discarding a ConflictError in the __aexit__. That is surprising to users and may lead to data loss. Also, there is not even a way to detect the conflict error in those cases.

codecov · 2021-07-29T17:17:03Z

Codecov Report

Merging #35 (ea3af0b) into master (e6afd5d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   96.67%   96.70%   +0.03%     
==========================================
  Files          11       11              
  Lines         841      851      +10     
==========================================
+ Hits          813      823      +10     
  Misses         28       28

Impacted Files	Coverage Δ
aiocouch/document.py	`92.52% <100.00%> (+0.45%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6afd5d...ea3af0b. Read the comment docs.

adeelsohailahmed · 2021-07-29T21:58:50Z

Thank you for your kind review!

May I suggest keeping the Database member functions as they are for more flexibility / finer control?

Can we think of Document / Security Document context managers as sort of an advanced feature? Due to the assumptions these context managers currently make, it's a tradeoff between convenience and finer control.

This is the sort of compromise that python-cloudant seems to have made as well.

(I'm not suggesting to follow python-cloudant for everything. I just love that, as a user, both aiocouch and python-cloudant are almost similar despite being very different under-the-hood).

As for ConflictError, my bad. Would this approach be alright (as shown in the example of aiocouch documentation)? Or should we simply let the ConflictError bubble up?

--- a/aiocouch/document.py
+++ b/aiocouch/document.py
@@ -97,7 +97,11 @@ class Document(RemoteDocument):
         traceback: Optional[TracebackType],
     ) -> None:
         if exc_type is None:
-            with suppress(ConflictError):
+            try:
+                await self.save()
+            except ConflictError:
+                info = await self.info()
+                self.rev = info["rev"]
                 await self.save()

adeelsohailahmed · 2021-08-04T22:25:38Z

@bmario Can you please spare some time to review the changes I've made?

adeelsohailahmed · 2021-08-05T13:55:03Z

I'm sorry, but I am at a loss here. I don't know why the Actions keep failing with Error: Resource not accessible by integration.

Mypy itself is not reporting any error:

{"name":"Mypy","head_sha":"e3c566051b46446098faf73f3ae72d778cb5f0fa","completed_at":"2021-08-05T12:30:00.907Z","conclusion":"success","output":{"title":"Mypy","summary":"There are 0 mypy warnings","annotations":[]}}

Am I missing something? Perhaps a token in my forked repo?

bmario

I still haven't figured out the interface yet. But having the context managers as part of the Document class seems fine for now.

bmario · 2021-08-05T13:49:45Z

aiocouch/document.py

+    ) -> None:
+        if exc_type is None:
+            try:
+                await self.save()


I think the exception should just bubble up the stack. The problem is that this solution to conflict resolution is just a guess for the correct conflict solution and it doesn't work either, as the second call to save might raise again.

bmario · 2021-08-05T13:55:41Z

tests/test_document.py

@@ -30,6 +30,26 @@ async def test_constructor_with_wrong_type_for_data(
        Document(database, "foo", data=cast(Any, doc))


+async def test_context_manager(database: Database) -> None:


Please split the test into two functions

I'll split this test into two functions with the following approach:

In the first function, I'll create and save a new document to the server using context manager. Then, I'll use the database methods to retrieve the newly created document and verify whether the data was actually written to the server.

The second function will be inverse of this. I'll create and save a new document using the database methods. After that, I'll use the context manager to retrieve the newly created document and verify the data.

tests/test_document.py

bmario · 2021-08-05T13:59:35Z

aiocouch/document.py

+        await self.fetch(discard_changes=True)
+        return self
+
+    async def __aexit__(


I think the implementation for this should be the same as in the parent class Document rendering this method redundant.

Yes, you're right. The __aexit__ method here is redundant indeed. I'll remove it. Thanks.

aiocouch/document.py

bmario · 2021-08-05T14:24:54Z

I'm sorry, but I am at a loss here. I don't know why the Actions keep failing with Error: Resource not accessible by integration.

Mypy itself is not reporting any error:
{"name":"Mypy","head_sha":"e3c566051b46446098faf73f3ae72d778cb5f0fa","completed_at":"2021-08-05T12:30:00.907Z","conclusion":"success","output":{"title":"Mypy","summary":"There are 0 mypy warnings","annotations":[]}}
Am I missing something? Perhaps a token in my forked repo?

Don't mind that. I think this is a GitHub Actions configuration issue on my part.

- use suppress to ignore NotFoundError - let ConflictError bubble up on Document __aexit__ - use typing.cast for Security Document __aenter__ - remove redundant Security __aexit__ - split context manager test into two functions - add test case for using context manager with data parameter

adeelsohailahmed · 2021-08-06T03:27:06Z

Thank you so much for taking the time to review the code! I've made changes as per your review.

bmario

The implementation itself looks good to me. I'd like to see the fixtures getting used in the one test case though.

Also, can you add a paragraph to the documentation about context managers? That would be appreciated.

bmario · 2021-08-06T07:30:43Z

tests/test_document.py

+    assert saved_doc["king"] == "elvis"
+
+
+async def test_context_manager_by_retrieving_existing_doc(database: Database) -> None:


Thanks for splitting the test. Though, I think this test should use one of the predefined documents of the filled_database fixture: https://github.com/metricq/aiocouch/blob/master/tests/conftest.py#L91

Updated the test to use the filled_database fixture.

- Use filled_database fixture for testing retrieving existing doc - Add missing `async` to Session document - Explain reasoning behind using async context managers - Give examples of Document & Security Document context managers

adeelsohailahmed · 2021-08-07T03:09:10Z

I'd like to see the fixtures getting used in the one test case though

I'm using the database fixture in every test case I've created (unless you were referring to using filled_database fixture).

Also, can you add a paragraph to the documentation about context managers? That would be appreciated.

I've added a section about context managers to the documentation. The section also contains appropriate, self-contained examples. Please take a look, and do let me know if there are some changes I ought to make.

I've also edited the session.rst. Its example was missing async.

bmario · 2021-08-09T08:28:06Z

Looks good to me now. Thank you.

adeelsohailahmed added 6 commits July 24, 2021 04:08

Add async context manager to Document;

69c2dd1

Add async context manager to Security Document;

Add tests for async context managers

30ecf96

(one each for Document & Security Document)

chore: remove extra line of code

bbcbdfa

Don't save doc on __aexit__ on unhandled exception

688c374

Don't use extra request to check if document exist

b14da6a

chore: remove unused auto-import

fb8edbc

Use new _rev on Conflict & Fix Mypy error

f917da5

bmario reviewed Aug 5, 2021

View reviewed changes

bmario reviewed Aug 6, 2021

View reviewed changes

Change test acc. to review & add documentation

ea3af0b

- Use filled_database fixture for testing retrieving existing doc - Add missing `async` to Session document - Explain reasoning behind using async context managers - Give examples of Document & Security Document context managers

bmario approved these changes Aug 9, 2021

View reviewed changes

bmario merged commit 563c6f7 into metricq:master Aug 9, 2021

adeelsohailahmed deleted the feat/context_managers_for_document_and_security branch August 9, 2021 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/Context Managers for Document and Security Document #35

Feat/Context Managers for Document and Security Document #35

adeelsohailahmed commented Jul 24, 2021

bmario commented Jul 29, 2021

codecov bot commented Jul 29, 2021 •

edited

Loading

adeelsohailahmed commented Jul 29, 2021

adeelsohailahmed commented Aug 4, 2021

adeelsohailahmed commented Aug 5, 2021

bmario left a comment

bmario Aug 5, 2021

bmario Aug 5, 2021

adeelsohailahmed Aug 6, 2021

bmario Aug 5, 2021

adeelsohailahmed Aug 6, 2021

bmario commented Aug 5, 2021

adeelsohailahmed commented Aug 6, 2021

bmario left a comment

bmario Aug 6, 2021 •

edited

Loading

adeelsohailahmed Aug 7, 2021

adeelsohailahmed commented Aug 7, 2021 •

edited

Loading

bmario commented Aug 9, 2021 •

edited

Loading

		@@ -30,6 +30,26 @@ async def test_constructor_with_wrong_type_for_data(
		Document(database, "foo", data=cast(Any, doc))


		async def test_context_manager(database: Database) -> None:

		assert saved_doc["king"] == "elvis"


		async def test_context_manager_by_retrieving_existing_doc(database: Database) -> None:

Feat/Context Managers for Document and Security Document #35

Feat/Context Managers for Document and Security Document #35

Conversation

adeelsohailahmed commented Jul 24, 2021

bmario commented Jul 29, 2021

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

adeelsohailahmed commented Jul 29, 2021

adeelsohailahmed commented Aug 4, 2021

adeelsohailahmed commented Aug 5, 2021

bmario left a comment

Choose a reason for hiding this comment

bmario Aug 5, 2021

Choose a reason for hiding this comment

bmario Aug 5, 2021

Choose a reason for hiding this comment

adeelsohailahmed Aug 6, 2021

Choose a reason for hiding this comment

bmario Aug 5, 2021

Choose a reason for hiding this comment

adeelsohailahmed Aug 6, 2021

Choose a reason for hiding this comment

bmario commented Aug 5, 2021

adeelsohailahmed commented Aug 6, 2021

bmario left a comment

Choose a reason for hiding this comment

bmario Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

adeelsohailahmed Aug 7, 2021

Choose a reason for hiding this comment

adeelsohailahmed commented Aug 7, 2021 • edited Loading

bmario commented Aug 9, 2021 • edited Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

bmario Aug 6, 2021 •

edited

Loading

adeelsohailahmed commented Aug 7, 2021 •

edited

Loading

bmario commented Aug 9, 2021 •

edited

Loading