Invalidation should trigger a call to getParameter to get a fresh context
strblr opened this issue · 9 comments
Hey,
For some context, I was facing the following bug: #980 (comment)
If I'm not mistaken, getParameter is not re-called when re-executing an invalidated query. This is I think the cause of the bug: dataloaders are instantiated when building the contextValue, so in getParameter. Thus, subsequent invalidation would make use of old dataloaders, with stale cached data in it. If any resolver depends on dataloaders, invalidation re-execution would use outdated data.
This seems like a major bug / limitation, dataloaders are very much needed in large apps. I'd suggest including the call to getParameter inside the routine that is launched on invalidation.
Hey @strblr, thank you for all the feedback so far ❤️
I wouldn't categorize it as a major bug as this is already how Subscriptions work all the time.
I definitely agree with you, that this is a major limitation.
We can allow passing a second (optional) function parameter to the makeExecute method here:
(execute: typeof defaultExecute, contextFactory: Context | (initialContext: Context) => PromiseOrValue<Context>) => What do you think? Would you be open to creating the PR for this?
Thank you for your quick response @n1ru4l, I'm glad if my feedback was useful !
If I understand your idea correctly, it would be to pass the initial contextValue to contextFactory here:
Like this:
[originalContextSymbol]: await contextFactory(contextValue),?
@n1ru4l This would make run async, thus changing a lot of typings, and maybe it will have other side-effects that I'm not aware off. Is this really a no-brainer?
I tried to find a workaround without changing the library. I don't understand why something like this doesn't work:
import { execute as defaultExecute, ExecutionArgs } from "graphql";
import { flowRight } from "lodash-es";
// ...
const live = new InMemoryLiveQueryStore();
const makeLazyContextExecute = (contextFactory: () => unknown) => (
execute: typeof defaultExecute
) => async (args: ExecutionArgs) =>
execute({ ...args, contextValue: await contextFactory() });
registerSocketIOGraphQLServer({
socketServer,
getParameter: async ({ graphQLPayload: { extensions } }) => ({
execute: flowRight(
applyLiveQueryJSONPatchGenerator,
flowRight(
makeLazyContextExecute(() => getContext(extensions?.authorization)),
live.makeExecute
)(defaultExecute)
),
graphQLExecutionParameter: { schema }
})
});
// getContext creates a new context with new dataloadersIt's a bit like the idea you suggested, only using external composition to create a special execute function.
In my understanding, this should totally discard the initial contextValue provided by graphQLExecutionParameter and replace it with a fresh call to getContext every time execute is called. But it still doesn't create a new context on invalidation. Do you have an idea why?
Thanks a lot!
Fixed it. There were two issues:
- Composition order:
makeLazyContextExecuteneeds to be wrapped insidelive.makeExecutefor it to be cached for invalidation, not the other way around. - Distinguishing between live and non-live queries. With live queries, the initial context is a
LiveQueryContextValuecontaining a symbol.
Although hacky, this works:
const makeLazyContextExecute = (
contextFactory: (previous: unknown) => unknown
) => (execute: typeof defaultExecute) => async (args: ExecutionArgs) =>
execute({ ...args, contextValue: await contextFactory(args.contextValue) });
registerSocketIOGraphQLServer({
socketServer,
getParameter: async ({ graphQLPayload: { extensions } }) => ({
execute: flowRight(
applyLiveQueryJSONPatchGenerator,
flowRight(
live.makeExecute,
makeLazyContextExecute(async previous => {
const ctx = await getContext(extensions?.authorization);
const sym =
isObject(previous) && Object.getOwnPropertySymbols(previous)[0];
return !sym ? ctx : { ...previous, [sym]: ctx };
})
)(defaultExecute)
),
graphQLExecutionParameter: { schema }
})
});(Exporting originalContextSymbol would make this solution cleaner)
I'll still try to PR the solution you suggested @n1ru4l
@strblr Yeah, I think the simpler API would be very convenient for library users! Looking forward to your PR!
Hi! Did anything come out of this? :)
Had a quick go at an envelop-implementation that keeps the API relatively simple for the user. Works for both live and non-live queries. Lemme know what you think!
Edit: It also implements patch delivery format based on context.
useLiveQuery.ts:
export interface UseLiveQueryOptions<Context extends Record<string, any>> {
applyLiveQueryPatchGenerator?: (
context: Context,
) => ReturnType<typeof createApplyLiveQueryPatchGenerator> | null;
contextFactory: (previous: Context) => unknown;
liveQueryStore: InMemoryLiveQueryStore;
}
const makeLazyContextExecute =
<Context extends Record<string, any>>(
contextFactory: (previous: Context) => unknown,
) =>
(execute: typeof defaultExecute) =>
makeStrongExecute(async (args) => {
const liveQuerySymbol =
isObject(args.contextValue) &&
Object.getOwnPropertySymbols(args.contextValue)[0];
if (liveQuerySymbol) {
const ctx = await contextFactory(args.contextValue[liveQuerySymbol]);
return execute({
...args,
contextValue: { ...args.contextValue, [liveQuerySymbol]: ctx },
});
} else {
const ctx = await contextFactory(args.contextValue);
return execute({
...args,
contextValue: ctx,
});
}
});
const makeWithPatchDeliveryExecute =
<Context extends Record<string, any>>(opts: UseLiveQueryOptions<Context>) =>
(execute: typeof defaultExecute) =>
makeStrongExecute(async (args: TypedExecutionArgs<Context>) => {
const applyLiveQueryPatchGenerator = opts.applyLiveQueryPatchGenerator
? opts.applyLiveQueryPatchGenerator(args.contextValue)
: undefined;
if (applyLiveQueryPatchGenerator) {
return pipe(args, execute, applyLiveQueryPatchGenerator);
}
return pipe(args, execute);
});
export const useLiveQuery = <Context extends Record<string, any>>(
opts: UseLiveQueryOptions<Context>,
): Plugin<Record<string, any>> => {
return {
onContextBuilding: ({ extendContext }) => {
extendContext({
liveQueryStore: opts.liveQueryStore,
});
},
onExecute: ({ executeFn, setExecuteFn }) => {
const execute = pipe(
executeFn,
makeLazyContextExecute(opts.contextFactory),
opts.liveQueryStore.makeExecute,
makeWithPatchDeliveryExecute(opts),
);
setExecuteFn(execute);
},
onValidate: ({ addValidationRule }) => {
addValidationRule(NoLiveMixedWithDeferStreamRule);
},
};
};makeStrongExecute.ts:
export function makeStrongExecute<ReturnType>(
weakExecute: (args: ExecutionArgs) => ReturnType,
) {
function execute(args: ExecutionArgs): ReturnType;
function execute(
schema: GraphQLSchema,
document: DocumentNode,
rootValue?: any,
contextValue?: any,
variableValues?: Maybe<{ [key: string]: any }>,
operationName?: Maybe<string>,
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>,
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>,
): ReturnType;
function execute(
argsOrSchema: ExecutionArgs | GraphQLSchema,
...otherArgs: any[]
): ReturnType {
if (argsOrSchema instanceof GraphQLSchema) {
return weakExecute({
contextValue: otherArgs[2],
document: otherArgs[0],
fieldResolver: otherArgs[5],
operationName: otherArgs[4],
rootValue: otherArgs[1],
schema: argsOrSchema,
typeResolver: otherArgs[6],
variableValues: otherArgs[3],
});
}
return weakExecute(argsOrSchema);
}
return execute;
}usage.ts:
function rescopeContext(context: Context) {
return { ...context, scope: v4() };
}
function getLiveQueryPatchGenerator(context: Context) {
const { patchDeliveryFormat } = context;
if (patchDeliveryFormat === PatchDeliveryFormat.JSONDIFFPATCH) {
return applyLiveQueryJSONDiffPatchGenerator;
}
if (patchDeliveryFormat === PatchDeliveryFormat.JSONPATCH) {
return applyLiveQueryJSONPatchGenerator;
}
return null;
}
useLiveQuery({
applyLiveQueryPatchGenerator: getLiveQueryPatchGenerator,
contextFactory: rescopeContext,
liveQueryStore,
}),