maragudk / goqite

Go queue library built on SQLite and inspired by AWS SQS.
https://maragudk.github.io/goqite/
MIT License
435 stars 13 forks source link

Report id of database row inserted by Send. #42

Closed austinjp closed 6 months ago

austinjp commented 6 months ago

Hi there. With reference to PR #41, it would be great if goqite could handle a particular use case I have.

I'm building a REST-ish API. Users submit jobs which are long-running (minutes/hours). Upon submission the API immediately responds with a '202 Accepted', and jobs are added to a goqite queue. According to received wisdom, the response sent to the user should include a 'Location' header with a URL where the user can check job progress, e.g. Location /task/123 where 123 is the ID of the job.

However, since the Send method of goqite.Queue does not return anything, I cannot reliably discern the ID of the item the user submitted. Therefore I can't build the /task/123 URL, and the user can't check progress.

I guess I could embed some identifying information into the job body, but this would need to be generated by the client whereas that's clearly a server-side responsibility. I'd also need to parse many large BLOBs in the database to look-up any item, which would be inefficient.

Consequently, I believe it would be useful if Send could return the ID of the row it inserts. This is trivial in SQL with a returning clause, supported in SQLite since version 3.35.0 (2021-03-12).

PR #41 provides this functionality. However, implementing it resulted in a cascade of changes throughout the code-base, which I appreciate isn't ideal. Happy to discuss alternatives! :smile:

markuswustenberg commented 6 months ago

Hi @austinjp . Thank you for the clarification! I think it makes sense to add something like this.

I'm a bit unsure about the API design, however. There are three main approaches I see:

I'll have to think about what approach I like the most. The last one is arguably the least idiomatic, but I kinda like it anyway. Hmm. I'll get back to this.

austinjp commented 6 months ago

Excellent, and no worries about taking time! I'm using my hacked version for now.

If it's of interest, my preference might also be your third option. When hacking on your code I initially returned an ID, then thought I'd prefer the Message instead in case of subsequent changes to the DB/schema/etc. Perhaps a Returning type might be useful, in addition to Message?

I'm going to close PR #41 because it contains a bug: the 'common' dir is inside the 'internal' dir, meaning the 'common' items are unavailable to code using my PR. I've fixed this in the latest commit to my fork.

markuswustenberg commented 6 months ago

@austinjp I'm trying out a change in #43. Will think about it a bit. I'm worried the pointer design would be too magic (non-obvious) for my taste.

austinjp commented 6 months ago

I'm worried the pointer design would be too magic (non-obvious) for my taste.

Yeah, I can see that. My thinking about returning an object (rather than an ID) was that it would hide the database implementation details to some degree, and both the object and the database schema could be extended or updated in the future without breaking changes (hopefully).

markuswustenberg commented 6 months ago

Okay, the pointer solution is in #44. Which one is the most intuitive to the caller?

Notes:

markuswustenberg commented 6 months ago

I'm worried the pointer design would be too magic (non-obvious) for my taste.

Yeah, I can see that. My thinking about returning an object (rather than an ID) was that it would hide the database implementation details to some degree, and both the object and the database schema could be extended or updated in the future without breaking changes (hopefully).

The ID is already used elsewhere. It's just an opaque string and not something like a UUID anyway, so I think that's fine.

austinjp commented 6 months ago

Which one is the most intuitive to the caller?

In all frankness, I may not be qualified to make that call :nerd_face: Coming from Python, I have little experience with pointers so #43 feels intuitive to me. But someone with experience of Go may prefer #44, so I have to plead ignorance :shrug:

Whichever you prefer! As long as I can meet my use-case, I'm happy :innocent:

Your responsiveness is much appreciated, by the way.

markuswustenberg commented 6 months ago

Just a quick update: I haven't forgotten about this, I just can't decide. 😅

austinjp commented 6 months ago

Just a quick update: I haven't forgotten about this, I just can't decide. 😅

No worries my dude, no rush. I haven't come to any firm conclusions myself :thinking:. I guess, in #43 the id, err := q.Send(...) approach looks like idiomatic Go to me, while the pointer does seem a tad magical :mage_man: as you say. Coming from Python, I'm a believer that explicit is better than implicit so on balance maybe I prefer #43.

markuswustenberg commented 6 months ago

Yeah, I like explicit better as well. I'm just a bit sad that in the arguably main send-and-forget approach, there's additional clutter on the client side, because the id won't be used in the common case: _, err := q.Send(…).

Maybe a SendAndGetThatIDBack would work better? :D

austinjp commented 6 months ago

Indeed!

Hmm. Public functions could be something like err := q.Send(…) and id, err := q.SendReturningID(…). This is nice because AFAICT this wouldn't be a breaking change to your API.

Internally, code duplication might be minimised by calling some 'private' third function e.g. q.SendOptionallyReturningID(…) which might use structs for func options and return values which would/wouldn't have an ID field as appropriate.

That way:

At this point, though, it's becoming about how best to hide clutter, and from whom! :laughing:

markuswustenberg commented 6 months ago

@austinjp third iteration in #45. :D

austinjp commented 6 months ago

I like it. Looks cleanest to me in terms of client-side clutter, API consistency, impact on code-base, etc.

markuswustenberg commented 6 months ago

@austinjp I renamed the method SendAndGetID, but otherwise I've stuck to the implementation you saw last week. Released in https://github.com/maragudk/goqite/releases/tag/v0.2.3 ! Thank you for all the feedback. 😊

austinjp commented 5 months ago

@markuswustenberg Fantastic, thanks for all your efforts! :clinking_glasses: