goqite icon indicating copy to clipboard operation
goqite copied to clipboard

Report id of database row inserted by Send.

Open austinjp opened this issue 1 year ago • 11 comments

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:

austinjp avatar May 08 '24 10:05 austinjp

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:

  • Make Send return a Message, like you've proposed in #41.
  • Make Send return an ID.
  • Have Send accept a pointer to a message instead, where the ID can be updated if the send succeeds.

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.

markuswustenberg avatar May 08 '24 10:05 markuswustenberg

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.

austinjp avatar May 08 '24 11:05 austinjp

@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.

markuswustenberg avatar May 08 '24 12:05 markuswustenberg

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).

austinjp avatar May 08 '24 12:05 austinjp

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

Notes:

  • The pointer solution doesn't create noise when calling Send, but the ID update feels a bit too magic
  • The ID solution fits nicely with passing ID's around in Extend/Delete, but will often not be used.

markuswustenberg avatar May 08 '24 12:05 markuswustenberg

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.

markuswustenberg avatar May 08 '24 12:05 markuswustenberg

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.

austinjp avatar May 08 '24 13:05 austinjp

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

markuswustenberg avatar May 21 '24 08:05 markuswustenberg

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.

austinjp avatar May 21 '24 09:05 austinjp

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

markuswustenberg avatar May 21 '24 09:05 markuswustenberg

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:

  • the new functionality is added
  • no breaking API changes (I think)
  • code duplication between q.Send(…) and q.SendReturningID(…) is minimal
  • client-side clutter is none/minimal
  • internal implementation can use whatever magic/non-magic you prefer

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

austinjp avatar May 22 '24 12:05 austinjp

@austinjp third iteration in #45. :D

markuswustenberg avatar May 24 '24 12:05 markuswustenberg

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

austinjp avatar May 24 '24 16:05 austinjp

@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. 😊

markuswustenberg avatar May 27 '24 08:05 markuswustenberg

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

austinjp avatar Jun 03 '24 16:06 austinjp