Skip to content

Commit

Permalink
More precise isMissingData for abstract type refinements
Browse files Browse the repository at this point in the history 
Summary:
== Background

The earlier version of runtime abstract refinement considered the type discriminator to be optional, since we didn't originally guarantee that discriminator would be fetched. That meant that we would sometimes consider data //not// missing when it was, and not suspend when we should have.

== This Diff

Now that the compiler/runtime ensures type discriminator fields are always fetched and processed (see previous diff in stack), **this diff changes the runtime to consider the type discriminator to be a required field**. That is, if the discriminator itself is missing we treat that as the query/fragment having missing data (and may refetch/suspend just like we would if any other field were missing).

This means that it is possible that we can have a query that //could// be fulfilled from cache except for type discriminators - e.g. there's a `... on SomeInterface { interface_field }` and we have `interface_field` in the cache but don't know whether the record implements `SomeInterface`. We may consider changing the way we handle that exact case in the future, but for now this approach is simple and predictable.

== Next Up

Note that there is one edge-case that is not handled yet (but is tested, see TODOs in TypeRefinement-test). The current logic is sufficient to correct set/reset `isMissing(Expected)Data` //within a single fragment//, but it doesn't work across fragment boundaries. Specifically, if we try to read a child fragment that was spread within an unmatched abstract refinement, data is not expected to be present: the parent refinement didn't match. But  we don't have a way to know that presently, and incorrectly consider data for the child missing. Again, see tests with TODO. This will be addressed in a follow-up by propagating the "we're inside an unmatched refinement" information down the tree via fragment refs.

Reviewed By: kassens

Differential Revision: D21589446

fbshipit-source-id: 229362034c33606755989328b3d62f6bb750e4ec
  • Loading branch information
@josephsavona @facebook-github-bot
josephsavona authored and facebook-github-bot committed May 20, 2020
1 parent 8649821 commit 08ba6f0
Show file tree
Hide file tree
Showing 4 changed files with 584 additions and 134 deletions.
32 changes: 24 additions & 8 deletions packages/relay-runtime/store/DataChecker.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,25 +301,30 @@ class DataChecker {
case INLINE_FRAGMENT: {
const {abstractKey} = selection;
if (abstractKey == null) {
// concrete type refinement: only check data if the type exactly matches
const typeName = this._mutator.getType(dataID);
if (typeName === selection.type) {
this._traverseSelections(selection.selections, dataID);
}
} else if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) {
// Abstract refinement, there are three cases:
// Abstract refinement: check data depending on whether the type
// conforms to the interface/union or not:
// - Type known to _not_ implement the interface: don't check the selections.
// - Type is known _to_ implement the interface: check selections.
// - Unknown whether the type implements the interface: check the selections,
// if a field is missing we don't know if it should exist or not, so we
// have to pessimistically assume the type _does_ implement the interface
// and treat those fields as missing.
// - Unknown whether the type implements the interface: don't check the selections
// and treat the data as missing; we do this because the Relay Compiler
// guarantees that the type discriminator will always be fetched.
const implementsInterface = this._mutator.getValue(
dataID,
abstractKey,
);
if (implementsInterface !== false) {
if (implementsInterface === true) {
this._traverseSelections(selection.selections, dataID);
}
} else if (implementsInterface == null) {
// unsure if the type implements the interface: data is
// missing so don't bother reading the fragment
this._handleMissing();
} // else false: known to not implement the interface
} else {
// legacy behavior for abstract refinements: always check even
// if the type doesn't conform
Expand Down Expand Up @@ -363,7 +368,18 @@ class DataChecker {
this._recordWasMissing = recordWasMissing;
break;
case TYPE_DISCRIMINATOR:
// We can ignore the type discriminator for checking
if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) {
const {abstractKey} = selection;
const implementsInterface = this._mutator.getValue(
dataID,
abstractKey,
);
if (implementsInterface == null) {
// unsure if the type implements the interface: data is
// missing
this._handleMissing();
} // else: if it does or doesn't implement, we don't need to check or skip anything else
}
break;
default:
(selection: empty);
Expand Down
99 changes: 61 additions & 38 deletions packages/relay-runtime/store/RelayReader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,42 +103,56 @@ class RelayReader {
// In this case, reset isMissingData back to false.
// Quickly skip this check in the common case that no data was
// missing or fragments on abstract types.
if (this._isMissingData) {
const record = this._recordSource.get(dataID);
if (record != null) {
const {abstractKey} = node;
if (abstractKey == null) {
const recordType = RelayModernRecord.getType(record);
if (recordType !== node.type && dataID !== ROOT_ID) {
// The record exists and its (concrete) type differs
// from the fragment's concrete type: data is
// expected to be missing, so don't flag it as such
// since doing so could incorrectly trigger suspense.
// NOTE `isMissingData` is short for "is missing
// *expected* data", and the data isn't expected here.
// Also note that the store uses a hard-code __typename
// for the root object, while fragments on the Query
// type will use whatever the schema names the Query type.
// Assume fragments read on the root object have the right
// type and trust isMissingData.
this._isMissingData = false;
}
} else if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) {
// Handle a related edge-case: if the fragment type is *abstract*,
// then data is only expected to be present if the type implements
// the interface (or is a member of the union). Reset isMissingData
// if the type is not known to implement the interface.
const implementsInterface = RelayModernRecord.getValue(
record,
abstractKey,
);
if (implementsInterface !== true) {
this._isMissingData = false;
}
}
const {abstractKey} = node;
const record = this._recordSource.get(dataID);
if (this._isMissingData === true && abstractKey == null && record != null) {
const recordType = RelayModernRecord.getType(record);
if (recordType !== node.type && dataID !== ROOT_ID) {
// The record exists and its (concrete) type differs
// from the fragment's concrete type: data is
// expected to be missing, so don't flag it as such
// since doing so could incorrectly trigger suspense.
// NOTE `isMissingData` is short for "is missing
// *expected* data", and the data isn't expected here.
// Also note that the store uses a hard-code __typename
// for the root object, while fragments on the Query
// type will use whatever the schema names the Query type.
// Assume fragments read on the root object have the right
// type and trust isMissingData.
this._isMissingData = false;
}
}

// Handle a related edge-case: if the fragment type is *abstract*.
if (
abstractKey != null &&
record != null &&
RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT
) {
// Abstract type refinement: if the fragment type is *abstract*,
// then data is only expected to be present if the type implements
// the interface (or is a member of the union), so there are 3
// cases we need to handle:
// - Type known to _not_ implement the interface: reset value for isMissingData.
// - Type is known _to_ implement the interface: don't reset value isMissingData.
// - Unknown whether the type implements the interface: treat the data as missing;
// we do this because the Relay Compiler guarantees that the type discriminator
// will always be fetched.
const implementsInterface = RelayModernRecord.getValue(
record,
abstractKey,
);
if (implementsInterface === false) {
// type is *known* to not implement the interface, so fields aren't
// expected to be present. reset isMissing(Expected)Data.
this._isMissingData = false;
} else if (implementsInterface == null) {
// we don't know if the type implements the interface or not, which
// constitutes data being missing
this._isMissingData = true;
} // else implementsInterface === true: we don't need to reset isMissingData
}

return {
data,
isMissingData: this._isMissingData,
Expand Down Expand Up @@ -207,18 +221,27 @@ class RelayReader {
this._traverseSelections(selection.selections, record, data);
}
} else if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) {
// abstract type refinement. similar to the top-level case,
// reset isMissingData if the type is not known to conform to the
// interface
// Abstract refinement: similar to the logic at the fragment root:
// - Type known to _not_ implement the interface: reset value for isMissingData.
// - Type is known _to_ implement the interface: don't reset value isMissingData.
// - Unknown whether the type implements the interface: treat the data as missing;
// we do this because the Relay Compiler guarantees that the type discriminator
// will always be fetched.
const isMissingData = this._isMissingData;
this._traverseSelections(selection.selections, record, data);
const implementsInterface = RelayModernRecord.getValue(
record,
abstractKey,
);
if (implementsInterface !== true) {
if (implementsInterface === false) {
// type doesn't implement the interface so these fields are
// not expected to be present.
this._isMissingData = isMissingData;
}
} else if (implementsInterface == null) {
// we don't know if the type implements the interface or not,
// which counts as something missing
this._isMissingData = true;
} // else implementsInterface === true: we don't need to reset isMissingData
} else {
// legacy behavior for abstract refinements: always read even
// if the type doesn't conform and don't reset isMissingData
Expand Down
4 changes: 2 additions & 2 deletions packages/relay-runtime/store/__tests__/DataChecker-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2149,7 +2149,7 @@ describe('check()', () => {
});
expect(target.size()).toBe(0);
});
it('returns `available` when an abstract refinement is only missing the discriminator field', () => {
it('returns `missing` when an abstract refinement is only missing the discriminator field', () => {
const {TestFragment} = generateAndCompile(`
fragment TestFragment on Query {
maybeNodeInterface {
Expand Down Expand Up @@ -2186,7 +2186,7 @@ describe('check()', () => {
defaultGetDataID,
);
expect(status).toEqual({
status: 'available',
status: 'missing',
mostRecentlyInvalidatedAt: null,
});
expect(target.size()).toBe(0);
Expand Down
Loading

0 comments on commit 08ba6f0

Please sign in to comment.