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

Add projection argument to Reference.fetch() #380

Merged
merged 13 commits into from
Sep 21, 2022

Conversation

whophil
Copy link
Collaborator

@whophil whophil commented Sep 16, 2022

Adds projection argument to Reference.fetch(), allowing partial reference documents to be returned.

Addresses half of issue #196

Copy link
Collaborator

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

I left a minor comment.

The issue you point to points to #98 which raises concerns about projection and caching mechanism. And the fact that DB names should be expected (unless umongo tries to translates field names into DB names but I think it wouldn't be the only place where DB names are needed).

In any case, you may merge this if you're happy with it. If we can't have a perfect solution, this is better than nothing. It would be nice if known limitations were documented, though.

umongo/frameworks/pymongo.py Outdated Show resolved Hide resolved
@whophil whophil marked this pull request as draft September 16, 2022 22:00
@whophil
Copy link
Collaborator Author

whophil commented Sep 16, 2022

Thanks @lafrech. I had actually not seen the discussion on issue #98. Let me think over this a bit and potentially rework.

@whophil whophil marked this pull request as ready for review September 17, 2022 06:00
@whophil
Copy link
Collaborator Author

whophil commented Sep 17, 2022

@lafrech I've implemented cook_find_projection, as suggested in #98, so that the projections can use uMongo field names and not DB names.

Other things discussed in #98:

  • The cached _document is just the first-fetched document, not a cache which is looked up by the projection. The projection-keyed cache may still be useful in the future. If implemented, it should have a configurable max size.
  • _reference_io_validate is untouched (it still uses .fetch()). I implemented Reference.exists for PyMongo and Motor, and modified the corresponding _reference_io_validate to use them. I did not implement for TxMongo, as I ran into some environment issues (see Import error with PyMongo==4.1.1. twisted/txmongo#278).

@whophil whophil marked this pull request as draft September 17, 2022 16:24
@whophil whophil marked this pull request as ready for review September 17, 2022 18:33
@whophil
Copy link
Collaborator Author

whophil commented Sep 17, 2022

FYI, the suggestion to not use.fetch() in _reference_io_validate makes a huge difference in performance in some cases.

In my application, I have a parent document which references over a thousand others. The time to .commit() the parent document went from 21.8 seconds to 1.5 seconds (PyMongo with mongo running on localhost).

Copy link
Collaborator

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

LGTM!

You may merge and release!

umongo/data_objects.py Outdated Show resolved Hide resolved
@whophil
Copy link
Collaborator Author

whophil commented Sep 20, 2022

@lafrech how do I make the CI "go" on this?

@lafrech
Copy link
Collaborator

lafrech commented Sep 20, 2022

That's a good question. I see no obvious reason. Hooks are enabled. Can't look into this right now.

Frankly, Azure CI is a nightmare to configure and rather than trying to fix this I'd be tempted to move to GitHub actions. You may give it a try in another PR if you want. Setting up tests is relatively straightforward. See what I did on flask-smorest. The tricky part would be launching MongoDB.


OK I peeked into Azure logs and CI did go but apparently didn't comment here with the results. Note sure if the pipeline logs there are public.

Here's the last result:

./tests/conftest.py:38:101: E501 line too long (106 > 100 characters)
./umongo/frameworks/motor_asyncio.py:415:101: E501 line too long (105 > 100 characters)

@whophil
Copy link
Collaborator Author

whophil commented Sep 21, 2022

OK I peeked into Azure logs and CI did go but apparently didn't comment here with the results. Note sure if the pipeline logs there are public.

I don't know where to find the link to these results, so I can't tell you if they're public or not :)

I fixed the too-long lines. Merging now!

As far as a release goes - what is the process on this project?

@whophil whophil merged commit 1b23dc7 into Scille:master Sep 21, 2022
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.

2 participants