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

add get_or_insert function to cache layer #673

Merged
merged 13 commits into from
Aug 6, 2024
Merged

Conversation

kaplanelad
Copy link
Contributor

@kaplanelad kaplanelad commented Jul 25, 2024

Adding the get_or_insert function to the cache driver for getting the option to get or insert in an easy way.

I have also added to test_cfg a dummy app context for testing

@kaplanelad kaplanelad changed the title add insert_or_get function to cache layer add get_or_insert function to cache layer Jul 25, 2024
src/cache/mod.rs Outdated

let result = app_ctx
.cache
.get_or_insert(get_key, &app_ctx, |_ctx: &AppContext| {
Copy link
Contributor

@jondot jondot Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume there is no way to capture ctx right? (this is why its passed to the function)
If possible to get something like this:

.get_or_insert("key", async || {
     Users.find(..&ctx).await
})

this would be the most realistic. notice that

  1. F must be async because to be realistic people would probably make async db calls there
  2. ctx if possible, should be captured to avoid passing it as a parameter just to "ship it" to F
  3. we might have a challenge with async closure (IIRC its unstable), so might need to see what other libs/frameworks do here

Capturing ctx would probably require some concept of sync and send, so might require wrapping ctx in Arc and/or Mutex

.get_or_insert(get_key, &app_ctx, |_ctx: &AppContext| {
Ok("loco-cache-value".to_string())
})
.get_or_insert(get_key, async { Ok("loco-cache-value".to_string()) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try a test where the async block captures something from app_ctx? like try a real DB call for example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added endpoint usage example + adding tests

@jondot jondot added this to the 0.7.0 milestone Aug 2, 2024
async fn get_or_insert(State(ctx): State<AppContext>) -> Result<Response> {
let res = ctx
.cache
.get_or_insert("user", async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something still seems off to me w/errors.

i believe, async block should just throw the regular Result from Loco (i.e. CacheError does not belong in that async block)

the block runs like any other "linear" code flow, only that in our case, the result of the block is cached. So the block can return a result or fail, like any other code flow. The failure should be for any original reason the block itself has failed.

a good way to look at it:

if you remove the cache completely, the code block should mostly stay the same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got you, changed for supporting it

@jondot jondot merged commit 1633208 into master Aug 6, 2024
13 checks passed
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