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

Context-switch? #26

Closed
hden opened this issue Dec 8, 2021 · 7 comments
Closed

Context-switch? #26

hden opened this issue Dec 8, 2021 · 7 comments

Comments

@hden
Copy link

hden commented Dec 8, 2021

When used with Lacinia, it is useful to allow a field resolver to modify the application context, with the change exposed just to the fields nested below, but to any depth.

(resolve/with-context result new-context)

In our specific use-case, we'd like to update a specific db parameter for just to the field below. However superlifter requires the parameter to be placed in its context. We are able to specify the initial context via the :urania-opts key, but what's the recommended way to update the urania env just to the fields nested below, but to any depth?

Refs:

@oliyh
Copy link
Owner

oliyh commented Dec 8, 2021

Hello,

Superlifter's operation is pretty disconnected from Lacinia's - this is the only way it can achieve the Dataloader functionality. Essentially you create a superlifter instance and throw fetches into it from Lacinia and it returns a promise that Lacinia understands, but it has no knowledge of resolvers or ancestry. The fetches are also performed on a thread pool so thread local bindings won't work either.

One way to achieve this is to alter the Lacinia context appropriately for child resolvers and make this an argument to your fetches, which are just records so can contain extra information for the fetch itself.

Can you think of any other ways?

Cheers

@hden
Copy link
Author

hden commented Dec 15, 2021

to alter the Lacinia context appropriately for child resolvers and make this an argument to your fetches

Yup. That's exactly what I'm trying. In my case the context in question is a Datomic DB (kind like a DB snapshot).

What do you think if I lazily create a bucket if there isn't one? I'll have to peek into superlifter's context. Is it acceptable usage?

(defn- has-bucket? [context bucket-id]
  (contains? @(get context :buckets)
             bucket-id))

(defn enqueue! [{:keys [superlifter]} {:keys [muse bucket-id env]}]
  (when-not (has-bucket? superlifter bucket-id)
    (s/add-bucket! superlifter bucket-id (assoc-in default-bucket-options
                                                   [:urania-opts :env]
                                                   env)))
  (s/enqueue! superlifter bucket-id muse))

@oliyh
Copy link
Owner

oliyh commented Dec 15, 2021

Looks generally ok, you need to be careful of two things:

  1. You have appropriate triggers on the bucket - if you know in advance when you create it what the size will be, that would be most efficient
  2. There is a race condition between you checking for the bucket and adding it - if another thread has created it and added a muse to it in the meantime, you may lose that muse. Ideally you would do this at the parent level before branching off into multiple child resolvers

Superlifter could make this a bit easier for you, either with a way to check the buckets or an atomic way of adding a bucket. Let me know what you settle on and we can see if there are some useful improvements to the superlifter API.

@hden
Copy link
Author

hden commented Dec 15, 2021

an atomic way of adding a bucket

Sounds cool.

@oliyh
Copy link
Owner

oliyh commented Feb 7, 2022

Reading the implementation of add-bucket! it does atomically check for a bucket with the same id so you won't lose any muse you've already enqueued there, but your new bucket won't be added (and it will emit a warning). So, you could just keep your code without the has-bucket? guard and the behaviour will be correct; you will however also get this warning that "bucket already exists" every time, which might be annoying.

@oliyh
Copy link
Owner

oliyh commented Apr 21, 2023

Hi @hden reading this now, superlifter has an atomic way to add a bucket, which would mean you don't need to check for the existence of one first.

If I've not misunderstood, are you happy for this to be closed?

Thanks

@hden
Copy link
Author

hden commented Apr 22, 2023

Sure. Thanks for reaching out.

@hden hden closed this as completed Apr 22, 2023
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