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

Jonyscathe/execute mutate #342

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jonyscathe
Copy link
Contributor

This adds support for modifying mock_data through any of:

session.execute(insert(my_table).values(<list of dicts to insert>))  # similar to session.add()
session.execute(insert(my_table), <list of dicts to insert>)  # similar to session.add()
session.execute(insert(my_table).returning(my_table), <list of dicts to insert>).scalar()  # similar to session.add(), in theory should return ORM object, in practice, don't quite have that one working yet... Not terribly simple to do, and not needed in my project.
session.execute(update(my_table).where(<some expression).values(<dict of values to update>))  # Will update data in self._mock_data as appropraite
session.execute(delete(my_table).where(<some expression))  # will delete data in self._mock_data as appropriate.

This allows for the mocking of ORM 2.0 style expressions .

Tests and documentation has not been updated yet, and I am not sure that this will not cause any runtime issues with sqlalchemy < 1.4.0.
I wanted to get feedback on this first before going to the effort to update tests/documentation/compatibility.

If you are happy with these changes then I'll update the tests/doco/etc.

Note: I currently have this working in a project that has 185 various session.execute() calls that previously had 185 various session.query() or session.add() calls. Mocking is working well, and the ability to actually update mocked data has been particularly useful as I wasn't able to do that previously.

Can now add data and read it back using session.execute(select(...))
For session.execute(update(...)) statements and
session.execute(delete(...)) statements
As an alternative to using session.add()
Still no tests cases or linting done on this, very much work in
progress. But it does seem to work ok
Manually throw in a boundary if an execute(insert()) is not
returning
Will hopefully finish this next week sometime
ORM 2.0 is pretty useable now.
Still needs tests
On no rows to update or delete
Undo changes I had made for testing
@jonyscathe
Copy link
Contributor Author

Any comments on whether or not this change is wanted?

It isn't a huge amount of work for me to get pre-commit passing, add tests, add documentation. But don't want to spend my time doing that if you aren't ok with the change itself.

Copy link
Owner

@rajivsarvepalli rajivsarvepalli left a comment

Choose a reason for hiding this comment

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

Thanks for the pull-request to add this feature.

One overall comment would be to add unit testing so that we can verify these changes and then we can collaborate on a separate PR to enhance documentation for this feature so that users can leverage this new feature.

Have you noticed any regression issues with these changes? It appears the current unit tests are passing so it looks these changes should function fine for existing users,

@@ -685,3 +703,100 @@ def _mutate_data(self, *args: Any, **kwargs: Any) -> Optional[int]:
temp_mock_data.append((calls, result))
self._mock_data = temp_mock_data
return num_deleted
# execute case
else:
Copy link
Owner

Choose a reason for hiding this comment

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

Is there an opportunity here to break to down this new logic into a new function?

Comment on lines 662 to 665
# TypeError indicates an old version of sqlalcemy that does not support
# executing select(table) statements.
# ArgumentError indicates a mocked table that cannot be selected, so
# cannot be mocked this way.
Copy link
Owner

Choose a reason for hiding this comment

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

For ArgumentError shouldn't we inform the user that the mocked table could not be selected?

For TypeError , can we write this logic to a statement that is supported by multiple SqlAlchemy versions or do a check to verify the version supports executing select(table) statements rather relying on a exception to catch this case?

mutate: Set[str] = {"add", "add_all", "delete"}
mutate: Set[str] = {"add", "add_all", "delete", "execute"}

mutate_and_unify: Set[str] = {"execute"}
Copy link
Owner

Choose a reason for hiding this comment

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

[FMI] Why is mutate and unify? Shouldn't this be mutate or unify?

Also do we need execute to be part of mutate as well?

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 point.
Execute should just be its own special case. Which can mutate data and potentially unify depending on what is being executed.

Also return data from an insert with a returning statement.
@jonyscathe
Copy link
Contributor Author

Thanks for the pull-request to add this feature.

One overall comment would be to add unit testing so that we can verify these changes and then we can collaborate on a separate PR to enhance documentation for this feature so that users can leverage this new feature.

Have you noticed any regression issues with these changes? It appears the current unit tests are passing so it looks these changes should function fine for existing users,

Added unit tests. Now back to 100% test coverage and pre-commit passing.
For tests I took test_mocking.py and made a similar file test_mocking_orm2.py and changed the code over to orm2 style and removed anything that was irrelevant or not supported.

I haven't added any support for session.get() or session.scalars(). See https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#migration-orm-usage for info.
scalars() would be fairly easy to add. get() would be harder as we would need to distinguish between something like session.query(User).get(42) and something like session.get(User, 42) which should both give the same result, but are quite different syntax.

For an insert that returns data, currently I have it returning all data for that table rather than what was just inserted... So technically it isn't quite right. Still thinking about how to make it only return the row(s) inserted in that instruction.

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.

None yet

2 participants