implement job listing #117

bgentry · 2023-12-16T23:31:21Z

This implements an API for listing jobs, including filters and cursor-based pagination. I've intentionally kept the options quite minimal to start, though the API design should expand nicely to support more sort and filter options. We will probably need a better way to generate query fragments for the cursor-based pagination if we bring in more options for that.

I recently upgraded sqlc locally and was getting diff here; thought it was best to just get us upgraded so I added a commit for that as well.

Currently based on #116 which I figured should be its own isolated PR.

bgentry · 2023-12-16T23:41:30Z

client.go

+	// TODO(bgentry): confirm with Brandur, do we want to error in this scenario?
+	// If so this error is not worded appropriately for these methods.
+	if c.driver.GetDBPool() == nil {
+		return nil, errInsertNoDriverDBPool
+	}


@brandur left this here to ask you. We will need to error for pseudo-drivers like the current database/sql one, right? Although this error seems specifically worded for insert. Should we make it more generalized?

So this error actually predates the database/sql driver slight. What it's trying to protect against is a case where you initiated a driver with a nil pool, but are trying to call a function that needs a pool. You might do this for example in a test case where you're only using test transactions:

https://riverqueue.com/docs/testing#test-transactions

But yeah, we can make the error more general to apply to non-insert functions like here.

I tweaked the name and text to be a little more generalized, lmk your thoughts.

brandur

Wicked! A few nits here, but I think it looks great and very close to mergeable.

Sorry about the delay on this — been doing a lot the last week and it's been a bit hard to concentrate, but will have more time next week.

.github/workflows/ci.yml

brandur · 2023-12-17T21:24:28Z

client.go

+	// TODO(bgentry): confirm with Brandur, do we want to error in this scenario?
+	// If so this error is not worded appropriately for these methods.
+	if c.driver.GetDBPool() == nil {
+		return nil, errInsertNoDriverDBPool
+	}


So this error actually predates the database/sql driver slight. What it's trying to protect against is a case where you initiated a driver with a nil pool, but are trying to call a function that needs a pool. You might do this for example in a test case where you're only using test transactions:

https://riverqueue.com/docs/testing#test-transactions

But yeah, we can make the error more general to apply to non-insert functions like here.

client.go

brandur · 2023-12-24T22:45:14Z

internal/dbadapter/db_adapter.go

+}
+
+type JobListParams struct {
+	Conditions string


Just checking here: conditions are meant to be strictly stay an internal construct right? Just wondering if we have to do anything about making sure that SQL injections aren't possible.

Yes indeed. The thought is we'll only expose it in ways that do not accept raw user input. Obviously it would be ideal if we had a fully flexible query builder to do this all in a safe way, but barring that we'll just need to be mindful when expanding use of this internal API to ensure we don't add any SQL injection opportunities. Or if we do that would have to be very clearly called out as dangerous, but hopefully we won't find a reason to need to do that.

brandur · 2023-12-24T22:47:33Z

job_list_params.go

+
+// UnmarshalText implements encoding.TextUnmarshaler to decode the cursor from
+// a previously marshaled string.
+func (c *JobListPaginationCursor) UnmarshalText(text []byte) error {


Just want to make sure I'm following: what's the purpose of this function? Is the idea here that it allows the cursor to be saved to JSON or otherwise persisted? Is that expected to be a feature that people want?

MarshalText is the fallback for when a type does not implement MarshalJSON. This allows it to be marshaled and unmarshaled cleanly as an opaque JSON string, one which River can then ingest to set the cursor for the next page query. The idea being that whatever params are used for sorting can be encoded into the cursor so that the pagination can efficiently resume at the next row.

In an API that powers a UI, for example, I'd expose the cursor so that the frontend can send it back with a subsequent request to fetch the next page.

job_list_params.go

brandur · 2023-12-24T22:50:39Z

internal/dbsqlc/job_list.go

@@ -0,0 +1,118 @@
+package dbsqlc


So I think my main (big) nit is mostly that this should live somewhere besides dbsqlc — I think that it's a little too confusing that this code looks like it's generated, but it's not generated, and that dbsqlc shares a mix of generated and non-generated code. IMO, we should put it somewhere else even if it looks almost exactly like dbsqlc code.

I moved this into db, not sure if you have anything better in mind. It's getting a little confusing with dbsqlc + dbadapter + db all in there 🤔

bgentry · 2024-01-03T15:45:15Z

@brandur freshly rebased, and expanded to allow for filtering to match a metadata fragment with .Metadata("{\"foo\": \"bar\"}"). I also made the state filter not required, though there still is a default value and you have to explicitly choose to clear it out with .State("").

It's definitely pretty ugly internally, but I think it works for these limited use cases. I'd like to clean it up at some point but I think that's lower priority given that the ugliness is well contained here.

brandur

Sorry for the delay on this review. Thx for the fixes.

client.go

internal/db/job_list.go

brandur · 2024-01-06T22:08:03Z

job_list_params.go

+const (
+	// JobListOrderByTime specifies that the sort should be by time. The specific
+	// time field used will vary by job state.
+	JobListOrderByTime JobListOrderByField = iota


Are you sure about this one? Wouldn't it be better just to have an option for each timestamp field that you'd specify explicitly for what to sort on?

Currently, you have to read source code to understand what this will order by, and even if it were to be documented, it might be faster/easier to have users explicitly specify the timestamp they want.

I'm reasonably sure that this is the behavior I want in the UI, though I could easily extract the logic that maps state to sort field into that external library. On the other hand, it'd be easy for us to add additional field-specific sort options in here as well. Part of the issue is that it doesn't necessarily make sense to sort by i.e. finalized_at when listing jobs that aren't yet finalized, so you'd get a bunch of jobs with NULLS FIRST at the beginning of the list. You could default to scheduled_at but that doesn't seem particularly relevant when listing jobs that have already completed.

Do you specifically think this behavior is bad and confusing to the extent that it shouldn't be included at all, or that we should have a different default, or better documentation, or just that we shouldn't ship this without additional options?

Do you specifically think this behavior is bad and confusing to the extent that it shouldn't be included at all, or that we should have a different default, or better documentation, or just that we shouldn't ship this without additional options?

Oh we should definitely have list ordering behavior included — I was thinking more the difference of making it explicit versus implicit. Implicit would be more like you have here where you tell it to sort by time and you get back certain behaviors depending on context. Explicit would be more like you specifically ask to sort on ScheduledAt, FinalizedAt, etc.

The documentation could be improved, but IMO a case could still be made that this can still be slightly suboptimal because you need to read the docs to know what it's going to do instead of it being pretty obvious.

I see what you mean that this is definitely a sensible thing from the UI's perspective, although I think there's a chance that this should behave more like a dumb ORM and require everything to be explicit from the programmatic API here.

All that said, I guess explicit order by fields could be added in the future and cohabitate peacefully with this one, so having it available isn't harmful. Not strongly against if you want to go this direction.

brandur

@bgentry Thanks for putting in those feedback changes. Let me know if you want to discuss that order thing more, but don't let me block you on this any further. Marking approved.

This implements an API for listing jobs, including filters and cursor-based pagination.

This is applicable to more than just inserts.

bgentry · 2024-01-14T21:38:59Z

Thanks, I suspect we'll revisit the sort params here as soon as people start using it. For now it at least lets me keep moving forward to get this merged.

bgentry requested a review from brandur December 16, 2023 23:31

bgentry force-pushed the bg-job-list branch from 2011ec0 to e33fcce Compare December 16, 2023 23:33

bgentry commented Dec 16, 2023

View reviewed changes

Base automatically changed from bg-remove-redundant-internal-rivertest to master December 17, 2023 20:58

bgentry mentioned this pull request Dec 19, 2023

fix ScheduledAt functionality with InsertManyTx #121

Merged

brandur reviewed Dec 24, 2023

View reviewed changes

bgentry force-pushed the bg-job-list branch from e33fcce to 6dcc4df Compare December 25, 2023 20:42

bgentry requested a review from brandur December 25, 2023 20:43

bgentry force-pushed the bg-job-list branch 4 times, most recently from b3a035c to cde2582 Compare January 3, 2024 15:44

brandur reviewed Jan 6, 2024

View reviewed changes

bgentry force-pushed the bg-job-list branch from cde2582 to c559df3 Compare January 8, 2024 02:00

bgentry requested a review from brandur January 8, 2024 02:01

brandur approved these changes Jan 13, 2024

View reviewed changes

bgentry added 5 commits January 14, 2024 15:25

implement job listing

cc50564

This implements an API for listing jobs, including filters and cursor-based pagination.

PR feedback refactoring

2a198f5

more generalized 'no db pool' error

0255f44

This is applicable to more than just inserts.

allow metadata filtering, don't require state, cleaner tests

a8fbda8

PR feedback

10e0a71

bgentry force-pushed the bg-job-list branch from c559df3 to 10e0a71 Compare January 14, 2024 21:32

bgentry merged commit 985fe1e into master Jan 14, 2024
7 checks passed

bgentry deleted the bg-job-list branch January 14, 2024 21:39

This was referenced Jan 16, 2024

API naming inconsistency #150

Closed

rename Client.Cancel to Client.JobCancel #152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement job listing #117

implement job listing #117

bgentry commented Dec 16, 2023 •

edited

Loading

bgentry Dec 16, 2023

brandur Dec 17, 2023

bgentry Dec 25, 2023

brandur left a comment

brandur Dec 17, 2023

brandur Dec 24, 2023

bgentry Dec 25, 2023

brandur Dec 24, 2023

bgentry Dec 25, 2023 •

edited

Loading

brandur Dec 24, 2023

bgentry Dec 25, 2023

bgentry commented Jan 3, 2024

brandur left a comment

brandur Jan 6, 2024

bgentry Jan 8, 2024

brandur Jan 13, 2024

brandur left a comment

bgentry commented Jan 14, 2024

implement job listing #117

implement job listing #117

Conversation

bgentry commented Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgentry Dec 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgentry commented Jan 3, 2024

brandur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur left a comment

Choose a reason for hiding this comment

bgentry commented Jan 14, 2024

bgentry commented Dec 16, 2023 •

edited

Loading

bgentry Dec 25, 2023 •

edited

Loading