BREAKING CHANGE: make using non-enforcing entity loader explicit

[wschurman]

Why

This has been discussed many many times and it's long overdue: it's time to make non-enforcing loads not the default. Whether we make enforcing loads the default is another discussion to be had, but is out of the scope of this PR.

This PR introduces a new required intermediate call to loaders to explicitly specify either enforcing or non-enforcing load behavior.

-    const entityResult = await TestEntity.loader(viewerContext).loadByIDAsync(id);
+    const entityResult = await TestEntity.loader(viewerContext)
+      .withAuthorizationResults()
+      .loadByIDAsync(id);

Note that .enforcing calls remain unchanged.

The reasoning behind this change is that it's too easy to misinterpret a Failure result to also be the result of the call when a database error is thrown from a non-enforcing method, when in fact it's only a Failure when the privacy policy evaluation fails.

In Expo's use of Entity, we almost exclusively use .enforcing(), to the point where we want to add a lint rule to discourage non-enforcing. Rather than go down this path, we figure it's better to just make the non-enforcing explicit. And then maybe in the future make enforcing default, though that's a ways in the future to avoid implicit breakage.

How reviewers can help

- Help decide on the name for .nonEnforcing. Note that the .enforcing method is considered the opposite. Other names I considered are .resultWrapped(), .asyncResult(), but I'm sure there are others. I went with .nonEnforcing since it seemed the clearest.

How

Add .withAuthorizationResults method to EntityLoader, add new AuthorizationResultBasedEntityLoader class, move methods there.

In a follow-up, I'll look into moving the common methods (the ones in knownLoaderOnlyDifferences) to a different utils class since it no longer makes sense for them not to be on the loader class. But to limit the scope of this PR, they are kept in the AuthorizationResultBasedEntityLoader class for now.

Test Plan

Run all tests.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (778d9e3) to head (5210fda).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #238   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        70    +1     
  Lines         1899      1912   +13     
  Branches       262       262           
=========================================
+ Hits          1899      1912   +13     

Flag	Coverage Δ
integration	100.00% <100.00%> (ø)
unittest	100.00% <100.00%> (ø)

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.

[wschurman]

Need to add to index.ts

[ide]

For naming: nonEnforcing() makes sense next to enforcing() but in isolation I think it would be confusing. I am thinking about an endgame state where loaders are enforcing by default without enforcing() and the whole concept of enforcing a value is less present.

People expect the default behavior to be that methods return entity objects and throw if there is an error, and the non-default behavior is to return a Result object. "Enforcing" that a value is present is a Result concept. So if Results are no longer the default, I think the concept of "enforcing" should also no longer be prominent in the API. I think this lines up with your thinking in the PR description.

Some possibilities:

• XEntity.loader(...).returnResults().loadByIDAsync(id)
• XEntity.loader(...).withResults().loadByIDAsync(id)
• XEntity.loader(...).loadResultByIDAsync(id)
• XEntity.resultLoader(...).loadByIDAsync(id)

To be comprehensive, I would not do XEntity.loader(..., { loadResults: boolean }).loadResultByIDAsync(id) because it is better for the loader type to be always statically analyzable.

It will also be helpful to document why a result-based loader is even desirable. The most compelling reason IMO is when loading multiple entities or, more generally, performing multiple operations and being able to return the successful status or error for each operation.

[quinlanj]

lgtm, I've never actually used a nonEnforcing entity result (I've always opted to use try-catch instead) so this is a welcome change.

No strong feelings about the naming, but I prefer nonEnforcing more than the other candidates

[quinlanj]

The reasoning behind this change is that it's too easy to misinterpret a Failure result to also be the result of the call when a database error is thrown from a non-enforcing method, when in fact it's only a Failure when the privacy policy evaluation fails.

I've always thought non enforcing methods just wrapped all the errors in a Failure result, didn't actually realize it was limited to just privacy policy evaluation. How about nonPolicyEnforcing then?

[ide]

Privacy policies are always enforced by entities, and "enforcing" in this context is unrelated to privacy policies. To me, this is an additional reason to avoid the word "enforcing" in the naming.

[wschurman]

The way non-enforcing works currently is that only privacy policy (and entity constructor) errors are wrapped in a result (see constructAndAuthorizeEntitiesArrayAsync). All other errors (db errors included) are thrown before constructAndAuthorizeEntitiesArrayAsync is reached. So enforcing is related to privacy policies in the sense that all errors are thrown.

I really like the idea of having the result-based sub-loader name indicate Result-based privacy and construction in the name. Maybe something like XEntity.loader(...).withAuthorizationResults().loadByIDAsync(id) or XEntity.loader(...).withResults().loadByIDAsync(id).

As for whether to keep enforcing() sub-loader or move the enforcing methods to the loader, let's hold off on deciding that for now. I think it may make sense to keep it as a sub-loader in the long run, but this isn't clear yet. For now we can't move them to the top-level unless we rename them since they'd be an more dangerous implicit breaking change for library consumers than I'm comfortable relying upon typescript to catch. Maybe in a few versions we can reassess.

I'll change this to withAuthorizationResults.

[ide]

Mixed feelings on the name but since it's such a secondary API, I'm ok with moving forward with it.

What I like is that it hints to the reader that not every error is returned in a Result. In contrast, Promise.allSettled always returns Results for all types of errors. That actually could be a good change for us to make but is a separate discussion.

What I don't like is that the name implies the return value has details about an authorization check. Setting aside Results for a moment, if someone says "authorization result" you'd think it'd be some data that contains PASS/FAIL state.

[ide]

Out of scope, but renaming this in the future could make sense. The word "enforcing" is confusing except when working with Result objects.

[wschurman]

What I like is that it hints to the reader that not every error is returned in a Result. In contrast, Promise.allSettled always returns Results for all types of errors. That actually could be a good change for us to make but is a separate discussion.

That's an interesting point. I actually had also forgotten that the Result type returned here was only for authorization until reading the code during this series of PRs. If and when we do change this so that all errors are wrapped (which is maybe what we should've done from the beginning), we'd want to have a new loader type, something like withResult so having this just be authorization error results is useful. One could imagine that in a different language result could be generic to the superclass of error (in addition to the successful type), so the loader could be something like withResult<AuthorizationError>().

What I don't like is that the name implies the return value has details about an authorization check. Setting aside Results for a moment, if someone says "authorization result" you'd think it'd be some data that contains PASS/FAIL state.

Yeah, I had the same gut feeling. My home though is that the Result type is clear enough to disambiguate the general term "authorization result" from what is meant here.