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

Report id of database row inserted by Send. #42

Closed
austinjp opened this issue May 8, 2024 · 15 comments
Closed

Report id of database row inserted by Send. #42

austinjp opened this issue May 8, 2024 · 15 comments

Comments

@austinjp
Copy link

austinjp commented May 8, 2024

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! 😄

@markuswustenberg
Copy link
Member

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
Copy link
Author

austinjp commented May 8, 2024

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 added a commit that referenced this issue May 8, 2024
The id is useful to interact with the message without receiving it first.

See #42
@markuswustenberg
Copy link
Member

@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 added a commit that referenced this issue May 8, 2024
The id is useful to interact with the message without receiving it first.

See #42
@austinjp
Copy link
Author

austinjp commented May 8, 2024

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
Copy link
Member

markuswustenberg commented May 8, 2024

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
Copy link
Member

markuswustenberg commented May 8, 2024

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
Copy link
Author

austinjp commented May 8, 2024

Which one is the most intuitive to the caller?

In all frankness, I may not be qualified to make that call 🤓 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 🤷

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

Your responsiveness is much appreciated, by the way.

@markuswustenberg
Copy link
Member

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

@austinjp
Copy link
Author

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 🤔. I guess, in #43 the id, err := q.Send(...) approach looks like idiomatic Go to me, while the pointer does seem a tad magical 🧙‍♂️ as you say. Coming from Python, I'm a believer that explicit is better than implicit so on balance maybe I prefer #43.

@markuswustenberg
Copy link
Member

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
Copy link
Author

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! 😆

markuswustenberg added a commit that referenced this issue May 24, 2024
The id is useful to interact with the message without receiving it first.

See #42
@markuswustenberg
Copy link
Member

@austinjp third iteration in #45. :D

@austinjp
Copy link
Author

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

markuswustenberg added a commit that referenced this issue May 27, 2024
The id is useful to interact with the message without receiving it
first.

See #42
@markuswustenberg
Copy link
Member

@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
Copy link
Author

austinjp commented Jun 3, 2024

@markuswustenberg Fantastic, thanks for all your efforts! 🥂

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

No branches or pull requests

2 participants