feat!: Move knex-specific loader methods to knexLoader

[wschurman]

Why

This is a long stack, but the goal is to make a place to put postgres-specific loading logic to avoid polluting the core dataloader-based library with logic specific to knex/postgres. And potentially even postgres-specific mutation logic but that's way down the road.

The best place for this stuff is in the entity-database-adapter-knex library. Therefore, the strategy is (in PR order):

1. Move existing knex/postgres-specific loader methods to a new loader, knexLoader. I'm also open to calling this a "Postgres Loader" since the entity-database-adapter-knex package already uses postgres-specific queries (and more are added here since this stack eventually produces raw queries) within knex even though technically knex is database agnostic.
2. Move knex-specific logic out of EntityDataManager, move it to its own EntityKnexDataManager.
3. Move these two now-separated pieces into the entity-database-adapter-knex package. This requires creating a "plugin" system for database adapters, where when they are included their methods are added to the prototype so that seamless loading continues to work.
4. Add a codemod for these three pieces.
5. Add the first new feature to entity loading, load by tagged template string, which creates a better/safer way to express raw queries for loading entities.

Future other things that could be added here (after thorough thought on authorization) are:

• Pagination
• Create, update, delete by tagged template string
• Batch creation
• Upsert (maybe)

For Reviewers

The most critical things to assess are:

• "install" method in feat!: move knex loaders into entity-database-adapter-knex package #410
• sql tagged template API usability in feat!: Add sql template and loader method #414

How

This is part 1. It refactors the loaders by moving loadManyByFieldEqualityConjunctionAsync, loadFirstByFieldEqualityConjunctionAsync, and loadManyByRawWhereClauseAsync to a separate loader.

The new pattern for these is:

await PostgresTestEntity.knexLoader(vc1).loadManyByRawWhereClauseAsync(...)

Test Plan

This has full coverage in new tests.

[wschurman]

• chore: move knex-specific loader logic out of EntityDataManager #408 : 2 dependent PRs (#409 , #410 )
• feat!: Move knex-specific loader methods to knexLoader #407 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (bc74562) to head (00bb4a7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##              main      #407    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           91        96     +5     
  Lines        12694     13144   +450     
  Branches      1101       664   -437     
==========================================
+ Hits         12694     13144   +450     

Flag	Coverage Δ
integration	6.98% <0.00%> (-0.25%)	⬇️
unittest	97.18% <100.00%> (+0.09%)	⬆️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ide]

Reorganizing the code so the Knex-related parts are closer together seems fine to me.

I don't think the knexLoader is necessary to aid LLMs, which are just as happy to read docblocks. Or we could add "Knex" to the names of the Knex methods.

The eventual refactoring in #410 seems partly off to me. Specifically, EntityPrivacyUtils moves to the knex package but to me it seems like a general concern, regardless of whether Knex is used. Maybe that is not the case.

I worry this refactoring is in a partway state where the lines of separation aren't well drawn and it would be better to export everything from the main entitty package and have the knex package be an internal hard dependency.

[wschurman]

The current abstraction is at the adapter layer. What has happened though is that SQL-specific (knex adapter-specific) logic has leaked into the main entity package (loadManyByRawWhereClause, for example). It was done this way thus far in order to avoid the dynamic extending of types and installation of methods since they're not technically as type-safe and type-safety is a core trait of this library.

What we're seeing is that the quantity of adapter-specific logic and types that are leaking into the main package is increasing. We added loadByFieldEqualityConjunction and loadFirstByFieldEqualityConjunction, both of which aren't specifically sql but usually just end up being sql. And we're wanting to add two more, loadManyBySQL and loadPageBySQL (in this stack).

The way I'm thinking about this is by hypothetically having N database adapter flavors already in existence. (We technically already do because of the unit test adapters, but not a production adapter, though there have been talks about having them for other databases.) To add loadPageBySQL, for instance, we'd need to build pagination into each adapter, which may be nontrivial for some non-relational databases. IMO it makes for a cleaner abstraction to allow adapters to provide logic specific to the way they load entities as long as they adhere to correct authorization and construction practices.

So, there's a couple options:

1. Keep the abstraction level as-is, allow SQL/knex-specific logic to exist in some form in the main package and require all database adapters to implement all methods. I'm not opposed to this, but I worry that as more and more methods are added, it'll become increasingly difficult to add a new database adapter for a different type of database.
2. Hoist the level of abstraction to be separate loaders for adapter-specific loads (what this PR and stack do). The benefit here is that the core required adapter methods remain intact for core cachable/dataloader functionality for field loads for any adapter. The line of separation is that dataloader and core cacheability (cache: true, composite field caching) remain in the main package, and any loading that requires adapter-specific constructs be in the adapter itself, which makes use of loader utils from the core package to authorize and construct. Maybe there's room here for a createLoader higher-order-function exported by the core package that does the authorization and construction?

EntityPrivacyUtils is an interesting case and you're right to point it out. It uses loadFirstByFieldEqualityConjunction, which isn't a core method as this stack defines it. But I do agree it's core functionality. It is only used when the edge is EntityEdgeDeletionAuthorizationInferenceBehavior.ONE_IMPLIES_ALL. Maybe the right call here is that this and other fieldEqualityConjunction methods are kept in the main package database adapter abstraction? Then, adapters that don't have a notion of "first" can throw in the concrete implementation and require using EntityEdgeDeletionAuthorizationInferenceBehavior.NONE. Or maybe the right call is to require all database adapter implementations to have a notion of "first"?

Re: LLMs - it's more about making things easier to lint and instruct. For example, I'd like to have a PR comment posting requesting EXPLAIN ANALYZE be run any time knexLoader is used rather than having to hardcode the methods. Also having a separate docblock for the loader itself can be useful to instruct LLMs to use caution (or check for indexes) rather than having to include that blurb for each separate loader method.

[ide]

That all makes sense.

For EntityPrivacyUtils: I think the notion of "one", as opposed to "first", makes sense for all databases. It's not a SQL/RDBMS concept. Some databases can implement "one" using "first". We would replace knexLoader.loadFirstByFieldEqualityConjunctionAsync with loader.loadOneByFieldEqualingAsync which has symmetry with loader.loadManyByFieldEqualingAsync.

[wschurman]

Merge activity

• Feb 9, 11:36 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Feb 9, 11:36 PM UTC: @wschurman merged this pull request with Graphite.