Skip to content

Commit

Permalink
More precise isMissingData for abstract type refinement
Browse files Browse the repository at this point in the history 
Summary:
Updates Relay runtime to take advantage of the new abstract type discriminator fields (`__isNode` etc). This behavior is enabled behind an off-by-default feature flag:

* ResponseNormalizer no longer traverse into abstract type refinements where the response type didn't match. Previously these selections would have been flattened and we would have tried to normalize them anyway, even though the server would not have evaluated those selections.
* Reader continues to try to //read// all selections. If the type is **not known to implement the interface** (the discriminator is missing or is `false`), then reset isMissingData.
* Checker uses the discriminator field to decide if data is missing or not. If the type is **known to not implement the interface** (discriminator is present and `false`), then don't check the selections. Otherwise check them  and don't reset recordWasMissing. The logic is that either the type does implement the interface and anything "missing" really is missing, or we don't know, and have to assume that the type does implement the interface.
* ReferenceMarker is intentionally *not* updated, since it shouldn't remove any data that Reader might try to read, and Reader always tries to read data regardless of whether the types conform to abstract refinements or not.

Note that this means there are situations where a component may legitimately be missing data, but we don't suspend bc we're missing the discriminator to prove definitely that data is missing. Overall this seems like the right strategy - note that checker() is more pessimistic and would consider data missing in this case.

Reviewed By: jstejada

Differential Revision: D21390159

fbshipit-source-id: c8b972da12133f6403c5d5ebe0470525933ef2b4
  • Loading branch information
@josephsavona @facebook-github-bot
josephsavona authored and facebook-github-bot committed May 7, 2020
1 parent f95b903 commit 1e0dba0
Show file tree
Hide file tree
Showing 8 changed files with 937 additions and 24 deletions.
26 changes: 23 additions & 3 deletions packages/relay-runtime/store/DataChecker.js
View file
Expand Up @@ -14,6 +14,7 @@
'use strict';

const RelayConcreteNode = require('../util/RelayConcreteNode');
const RelayFeatureFlags = require('../util/RelayFeatureFlags');
const RelayModernRecord = require('./RelayModernRecord');
const RelayRecordSourceMutator = require('../mutations/RelayRecordSourceMutator');
const RelayRecordSourceProxy = require('../mutations/RelayRecordSourceProxy');
Expand Down Expand Up @@ -296,16 +297,35 @@ class DataChecker {
this._traverseSelections(selection.selections, dataID);
}
break;
case INLINE_FRAGMENT:
if (selection.abstractKey == null) {
case INLINE_FRAGMENT: {
const {abstractKey} = selection;
if (abstractKey == null) {
const typeName = this._mutator.getType(dataID);
if (typeName != null && typeName === selection.type) {
if (typeName === selection.type) {
this._traverseSelections(selection.selections, dataID);
}
} else if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) {
// Abstract refinement, there are three cases:
// - 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.
const implementsInterface = this._mutator.getValue(
dataID,
abstractKey,
);
if (implementsInterface !== false) {
this._traverseSelections(selection.selections, dataID);
}
} else {
// legacy behavior for abstract refinements: always check even
// if the type doesn't conform
this._traverseSelections(selection.selections, dataID);
}
break;
}
case LINKED_HANDLE:
// Handles have no selections themselves; traverse the original field
// where the handle was set-up instead.
Expand Down
69 changes: 52 additions & 17 deletions packages/relay-runtime/store/RelayReader.js
View file
Expand Up @@ -12,6 +12,7 @@

'use strict';

const RelayFeatureFlags = require('../util/RelayFeatureFlags');
const RelayModernRecord = require('./RelayModernRecord');

const invariant = require('invariant');
Expand Down Expand Up @@ -102,26 +103,42 @@ 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 && node.abstractKey == null) {
if (this._isMissingData) {
const record = this._recordSource.get(dataID);
if (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 really more "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;
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;
}
}
}
}

return {
data,
isMissingData: this._isMissingData,
Expand Down Expand Up @@ -181,16 +198,34 @@ class RelayReader {
this._traverseSelections(selection.selections, record, data);
}
break;
case INLINE_FRAGMENT:
if (selection.abstractKey == null) {
case INLINE_FRAGMENT: {
const {abstractKey} = selection;
if (abstractKey == null) {
// concrete type refinement: only read data if the type exactly matches
const typeName = RelayModernRecord.getType(record);
if (typeName != null && typeName === selection.type) {
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
const isMissingData = this._isMissingData;
this._traverseSelections(selection.selections, record, data);
const implementsInterface = RelayModernRecord.getValue(
record,
abstractKey,
);
if (implementsInterface !== true) {
this._isMissingData = isMissingData;
}
} else {
// legacy behavior for abstract refinements: always read even
// if the type doesn't conform and don't reset isMissingData
this._traverseSelections(selection.selections, record, data);
}
break;
}
case FRAGMENT_SPREAD:
this._createFragmentPointer(selection, record, data);
break;
Expand Down
19 changes: 17 additions & 2 deletions packages/relay-runtime/store/RelayResponseNormalizer.js
View file
Expand Up @@ -12,6 +12,7 @@

'use strict';

const RelayFeatureFlags = require('../util/RelayFeatureFlags');
const RelayModernRecord = require('./RelayModernRecord');
const RelayProfiler = require('../util/RelayProfiler');

Expand Down Expand Up @@ -187,16 +188,30 @@ class RelayResponseNormalizer {
this._traverseSelections(selection, record, data);
}
break;
case INLINE_FRAGMENT:
if (selection.abstractKey == null) {
case INLINE_FRAGMENT: {
const {abstractKey} = selection;
if (abstractKey == null) {
const typeName = RelayModernRecord.getType(record);
if (typeName === selection.type) {
this._traverseSelections(selection, record, data);
}
} else if (RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT) {
const implementsInterface = data.hasOwnProperty(abstractKey);
RelayModernRecord.setValue(
record,
abstractKey,
implementsInterface,
);
if (implementsInterface) {
this._traverseSelections(selection, record, data);
}
} else {
// legacy behavior for abstract refinements: always normalize even
// if the type doesn't conform
this._traverseSelections(selection, record, data);
}
break;
}
case LINKED_HANDLE:
case SCALAR_HANDLE:
const args = selection.args
Expand Down
143 changes: 141 additions & 2 deletions packages/relay-runtime/store/__tests__/DataChecker-test.js
View file
Expand Up @@ -2016,8 +2016,7 @@ describe('check()', () => {
const {TestFragment} = generateAndCompile(`
fragment TestFragment on Query {
maybeNodeInterface {
# This "... on Node { id }" selection would be generated if not
# present, and is flattened since Node is abstract
# This "... on Node { id }" selection would be generated if not present
... on Node { id }
... on NonNodeNoID {
name
Expand All @@ -2034,6 +2033,7 @@ describe('check()', () => {
'client:root:maybeNodeInterface': {
__id: 'client:root:maybeNodeInterface',
__typename: 'NonNodeNoID',
__isNode: false,
name: 'Alice',
},
};
Expand Down Expand Up @@ -2096,4 +2096,143 @@ describe('check()', () => {
});
expect(target.size()).toBe(0);
});

describe('with feature ENABLE_PRECISE_TYPE_REFINEMENT', () => {
const RelayFeatureFlags = require('../../util/RelayFeatureFlags');

beforeEach(() => {
RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT = true;
});
afterEach(() => {
RelayFeatureFlags.ENABLE_PRECISE_TYPE_REFINEMENT = false;
});

it('returns `missing` when a Node record is missing an id', () => {
const {TestFragment} = generateAndCompile(`
fragment TestFragment on Query {
maybeNodeInterface {
# This "... on Node { id }" selection would be generated if not present
... on Node { id }
... on NonNodeNoID {
name
}
}
}
`);
const data = {
'client:root': {
__id: 'client:root',
__typename: 'Query',
maybeNodeInterface: {__ref: '1'},
},
'1': {
__id: '1',
__typename: 'User',
__isNode: true,
name: 'Alice',
// no `id` value
},
};
const source = RelayRecordSource.create(data);
const target = RelayRecordSource.create();
const status = check(
source,
target,
createNormalizationSelector(TestFragment, 'client:root', {}),
[],
null,
defaultGetDataID,
);
expect(status).toEqual({
status: 'missing',
mostRecentlyInvalidatedAt: null,
});
expect(target.size()).toBe(0);
});
it('returns `available` when an abstract refinement is only missing the discriminator field', () => {
const {TestFragment} = generateAndCompile(`
fragment TestFragment on Query {
maybeNodeInterface {
# This "... on Node { id }" selection would be generated if not present
... on Node { id }
... on NonNodeNoID {
name
}
}
}
`);
const data = {
'client:root': {
__id: 'client:root',
__typename: 'Query',
maybeNodeInterface: {__ref: '1'},
},
'1': {
__id: '1',
__typename: 'User',
// __isNode: true, // dont know if it implements Node or not
name: 'Alice',
id: '1',
},
};
const source = RelayRecordSource.create(data);
const target = RelayRecordSource.create();
const status = check(
source,
target,
createNormalizationSelector(TestFragment, 'client:root', {}),
[],
null,
defaultGetDataID,
);
expect(status).toEqual({
status: 'available',
mostRecentlyInvalidatedAt: null,
});
expect(target.size()).toBe(0);
});

it('returns `available` when a record is only missing fields in non-implemented interfaces', () => {
const {TestFragment} = generateAndCompile(`
fragment TestFragment on Query {
maybeNodeInterface {
# This "... on Node { id }" selection would be generated if not present
... on Node { id }
... on NonNodeNoID {
name
}
}
}
`);
const data = {
'client:root': {
__id: 'client:root',
__typename: 'Query',
maybeNodeInterface: {__ref: '1'},
},
'1': {
__id: '1',
__typename: 'NonNodeNoID',
__isNode: false,
// no 'id' bc not a Node
name: 'Not a Node!',
},
};
const source = RelayRecordSource.create(data);
const target = RelayRecordSource.create();
const status = check(
source,
target,
createNormalizationSelector(TestFragment, 'client:root', {}),
[],
null,
defaultGetDataID,
);
expect(status).toEqual({
status: 'available',
mostRecentlyInvalidatedAt: null,
});
expect(target.size()).toBe(0);
});
});
});

0 comments on commit 1e0dba0

Please sign in to comment.