feat: require explicit query context specification

[wschurman]

Why

This is a RFC.

The proposal is to require the queryContext argument to reduce the likelihood of accidental omittance within a transaction block in application code.

At Expo, we've noticed sometimes a transaction will be started and the loader/mutator calls within it may accidentally not include the query context (which must be explicitly supplied as an arg to run within the transaction):

await viewerContext.runInTransactionAsync(async (queryContext) => {
  const existingNote = await NoteEntity.loader(viewerContext) // note how queryContext was accidentally omitted but tsc didn't have an error
    .enforcing()
    .loadByIDAsync(args.id);
  return await NoteEntity.updater(existingNote, queryContext)
    .setField('title', args.note.title)
    .setField('body', args.note.body)
    .enforceUpdateAsync();
});

Closes ENG-11824.

How

The proposed change is to always require queryContext. At Expo, we added a method to our ViewerContext subclass to vend the default EntityQueryContext (non-transactional) for our default database adaptor flavor. I added a similar method to TestViewerContext within this repo to demonstrate the convenience accessor.

RFC

• Does this pattern make sense?
• Is the default query context convenience pattern appropriate?

Test Plan

yarn tsc

[linear]

ENG-11824 Investigate QueryContext required

[codecov]

Codecov Report

Attention: Patch coverage is 94.33962% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 95.36%. Comparing base (9c43a56) to head (7e40a52).
Report is 1 commits behind head on main.

Files	Patch %	Lines
..._integration-tests__/entities/TestViewerContext.ts	66.66%	2 Missing ⚠️
packages/entity-example/src/routers/notesRouter.ts	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
- Coverage   95.46%   95.36%   -0.11%     
==========================================
  Files          79       80       +1     
  Lines        2074     2069       -5     
  Branches      258      266       +8     
==========================================
- Hits         1980     1973       -7     
+ Misses         93       90       -3     
- Partials        1        6       +5     

Flag	Coverage Δ
integration	95.36% <94.33%> (-0.11%)	⬇️
unittest	95.36% <94.33%> (-0.11%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ide]

I'm open to this and think it probably is good to make the query context explicit.

How independent are VCs and QCs? Should VCs no longer have a getQueryContextForDatabaseAdaptorFlavor() method if application code that works with VCs will always have a QC anyway since VCs and QCs are now passed around together?

What are the potential consequences if a VC is passed in with a QC, but vc.getQueryContextForDatabaseAdaptorFlavor() !== qc?

Should we pair together VCs and QCs in an object like: interface EntityContext { viewerContext; queryContext; }? (I'm thinking probably no for now. Keep the values flatter.)

[quinlanj]

I'd be very happy with something like this in www as I'm always super paranoid of forgetting to pass a qc.

[wschurman]

How independent are VCs and QCs? Should VCs no longer have a getQueryContextForDatabaseAdaptorFlavor() method if application code that works with VCs will always have a QC anyway since VCs and QCs are now passed around together?

They are related in that a ViewerContext is the entry point into the entity framework from an application (it has the reference to the EntityCompanionProvider which is the "instance" of the entity framework). So for an application to get a vended QueryContext, it must go through the viewer context by definition. But they are not tied together and are independent. There can be many flavors of viewer context (depending on the application) and both transactional and non-transactional query contexts being used for any loader/mutator call at any point.

What are the potential consequences if a VC is passed in with a QC, but vc.getQueryContextForDatabaseAdaptorFlavor() !== qc?

I think maybe we should rename ViewerContext.getQueryContextForDatabaseAdaptorFlavor to ViewerContext.getNonTransactionalQueryContextForDatabaseAdaptorFlavor or something. In the PostgresEntityQueryContextProvider, this is just a reference to the global (singleton) knex instance.

So, to answer your question, all non-transactional query context (EntityNonTransactionalQueryContext) are equivalent in that they just forward the calls to the knex singleton (for postgres). TransactionalQueryContext though are unique and reference a single transaction; they're vended via runInTransactionAsync((vendedQueryContext) => ...

Should we pair together VCs and QCs in an object like: interface EntityContext { viewerContext; queryContext; }? (I'm thinking probably no for now. Keep the values flatter.)

I'd say no for now, but if/when we have more database adaptor flavors we may need to reassess. But even then, one could imagine just having single instance of a viewer context being passed around and then multiple query contexts being used for multiple databases. This is all theoretical though.

[wschurman]

Reverted due to the integration of this into our first-party codebase (www) being too unwieldy (6k tsc errors). Going to come up with a different approach, maybe something more codemod-able.