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

use new unique jobs implementation #38

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

bgentry
Copy link

@bgentry bgentry commented Jan 12, 2025

This moves the library to use the new unique jobs implementation from riverqueue/river#590 and migrates the sqlalchemy driver to use a unified insertion path, allowing bulk inserts to use unique jobs.

It's intended to be analogous to riverqueue/riverqueue-ruby#32.

Outstanding issues:

  • There's one failing test that I think is due to some mocking related issue and I haven't yet been able to figure it out. Maybe you'd be able to take a crack at that?
  • I think it also needs more comprehensive test coverage for the various unique options
  • by_args needs support for partial keys, and needs to use sorted json before hashing. Tests are essential for this.
  • There are a couple of type misalignment issues where I'm wanting to insert null values into the unique_key or unique_states fields, because sqlc generates code that doesn't allow None values in these input lists. I'm not seeing any clear way to resolve that.

@bgentry bgentry requested a review from brandur January 12, 2025 19:57
brandur added a commit that referenced this pull request Jan 25, 2025
This one's related to #38. It turns out that while trying to mock a
context manager kind of works, it will do the wrong thing in edge cases
like when an exception is thrown from inside a `with` block, silently
swallowing it and causing a return that's completely wrong.

There may be some way to fix the mock to make it do the right thing, but
instead of getting fancier with these mocks that are already awful,
instead repair the problem by defining a plain class that implements
context manager and just use that.
brandur added a commit that referenced this pull request Jan 25, 2025
This one's related to #38. It turns out that while trying to mock a
context manager kind of works, it will do the wrong thing in edge cases
like when an exception is thrown from inside a `with` block, silently
swallowing it and causing a return that's completely wrong.

There may be some way to fix the mock to make it do the right thing, but
instead of getting fancier with these mocks that are already awful,
instead repair the problem by defining a plain class that implements
context manager and just use that.
brandur added a commit that referenced this pull request Jan 25, 2025
This one's related to #38. It turns out that while trying to mock a
context manager kind of works, it will do the wrong thing in edge cases
like when an exception is thrown from inside a `with` block, silently
swallowing it and causing a return that's completely wrong.

There may be some way to fix the mock to make it do the right thing, but
instead of getting fancier with these mocks that are already awful,
instead repair the problem by defining a plain class that implements
context manager and just use that.
brandur added a commit that referenced this pull request Jan 25, 2025
This one's related to #38. It turns out that while trying to mock a
context manager kind of works, it will do the wrong thing in edge cases
like when an exception is thrown from inside a `with` block, silently
swallowing it and causing a return that's completely wrong.

There may be some way to fix the mock to make it do the right thing, but
instead of getting fancier with these mocks that are already awful,
instead repair the problem by defining a plain class that implements
context manager and just use that.
@brandur
Copy link
Contributor

brandur commented Jan 25, 2025

There's one failing test that I think is due to some mocking related issue and I haven't yet been able to figure it out. Maybe you'd be able to take a crack at that?

This took an absurdly long time to figure out, but the problem was that the context manager mock to return an executor was swallowing a thrown AssertionError in this with block:

    def insert_many(self, args: List[JobArgs | InsertManyParams]) -> list[InsertResult]:

        with self.driver.executor() as exec:
            return self._insert_many_exec(exec, args)

Thereby causing insert_many to return None and then insert fail as it tried to index element [0].

I put in a fix in #39 which seems to work. Take a look at that, we can merge it, and then it should fix the tests over here.

Medium term I want to strip all these mocks out. They always seem like a good idea, but every time I use them I remember why they're so painful to work with.

I'm going to time out on this PR for the day since I already spent too much time on it, but if you get blocked on one of your other work streams, maybe see if you can start looking into your no. 2 and no. 3 checkboxes.

There are a couple of type misalignment issues where I'm wanting to insert null values into the unique_key or unique_states fields, because sqlc generates code that doesn't allow None values in these input lists. I'm not seeing any clear way to resolve that.

Didn't get a chance to look at this one, but it's possible it'll require an additional sql parameter like unique_states_is_null that gets based into the SQL query.

brandur added a commit that referenced this pull request Jan 25, 2025
This one's related to #38. It turns out that while trying to mock a
context manager kind of works, it will do the wrong thing in edge cases
like when an exception is thrown from inside a `with` block, silently
swallowing it and causing a return that's completely wrong.

There may be some way to fix the mock to make it do the right thing, but
instead of getting fancier with these mocks that are already awful,
instead repair the problem by defining a plain class that implements
context manager and just use that.
@bgentry bgentry force-pushed the bg-newer-unique-jobs branch from c65e79f to 3899985 Compare January 29, 2025 16:11
This moves the library to use the new unique jobs implementation from
riverqueue/river#590 and migrates the sqlalchemy
driver to use a unified insertion path, allowing bulk inserts to use
unique jobs.
@bgentry bgentry force-pushed the bg-newer-unique-jobs branch from 3899985 to 31a2cd1 Compare January 29, 2025 16:14
@bgentry
Copy link
Author

bgentry commented Jan 29, 2025

@brandur nice, looks like #39 fixed the test issues. I rebased and now everything is passing from make test 🙌

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