Skip to content

Commit

Permalink
Enable noUncheckedIndexedAccess
Browse files Browse the repository at this point in the history 
When reviewing some code in this repo, I noticed some potential bugs (or at
least non-ideal error-handling cases) that arose from TypeScript's default
behavior of assuming that all index operations yield valid values. That is, by
default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y`
rather than `Y | undefined`, and it assumes that all array access attempts are
in bounds.

noUncheckedIndexedAccess changes this so that you generally need to actually
make sure that your indexes worked. (This is a bit awkward for arrays because it
doesn't let you rely on `.length` checks, but it's not the end of the world.)
More details on the flag are available at
microsoft/TypeScript#39560

Here's an overview of the sorts of changes involved:

- One of the issues tracked in #624 is that buildComposedSchema could fail with
  unclear errors if the input schema doesn't declare directives correctly; the
  code assumed that any `@join__owner` or `@join__type` directive application
  *must* have a non-null `graph` argument whose value matches one of the
  `join__Graph` enums. We probably should validate the definitions, but in the
  meantime this PR validates that the value we get out of the directive
  application is good, with explicit errors if not.
- Our code uses a lot of record types like `{[x: string]:
  Y}`. noUncheckedIndexedAccess means we need to actually check that the value
  exists. (Another alternative would be to change to `Map` which does not have
  this issue.) I added a helper `getOrSetRecord` which allowed us to express a
  very common case more succinctly, which also led to less duplication of code.
- In some cases where it is very clear from context that a lookup must
  succeed (eg, a length check), I just used `!`.
- Some cases in composition explicitly checked to see if directives had
  arguments but not if it had at least one argument; adding a `?.[0]` helped
  here.
- Many cases were fixed by just using `?.` syntax or saving a lookup in a
  variable before moving on.
- Iteration over `Object.keys` could be replaced by `Object.values` or
  `Object.entries` to eliminate the index operation.
- In some cases we could change the declaration of array types to be "non-empty
  array" declarations, which look like `[A, ...A[]]`. For example, the groupBy
  operation always makes the values of the returned map be non-empty arrays. I
  was also able to replace a bunch of the FieldSet types in the query planner
  with a NonemptyFieldSet type; there are other places where it might be good to
  improve our checks for nonempty FieldSets though.
- Nested `function`s that close over values in their surrounding scope can't
  rely on narrowing of those values, but arrow functions assigned to a `const`
  that happen after the narrowing can, so in some cases I replaced nested
  functions with arrow functions (which generally involved moving it around
  too).
  • Loading branch information
@glasser
glasser committed Apr 9, 2021
1 parent 608d008 commit 123303f
Show file tree
Hide file tree
Showing 25 changed files with 353 additions and 292 deletions.
19 changes: 9 additions & 10 deletions federation-integration-testsuite-js/src/fixtures/accounts.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,22 +123,21 @@ export const resolvers: GraphQLResolverMap<any> = {
: user.birthDate;
},
metadata(object) {
const metaIndex = metadata.findIndex(m => m.id === object.id);
return metadata[metaIndex].metadata.map(obj => ({ name: obj.name }));
return metadata
.find((m) => m.id === object.id)
?.metadata.map((obj) => ({ name: obj.name }));
},
},
UserMetadata: {
address(object) {
const metaIndex = metadata.findIndex(m =>
m.metadata.find(o => o.name === object.name),
);
return metadata[metaIndex].metadata[0].address;
return metadata.find((m) =>
m.metadata.find((o) => o.name === object.name),
)?.metadata[0]?.address;
},
description(object) {
const metaIndex = metadata.findIndex(m =>
m.metadata.find(o => o.name === object.name),
);
return metadata[metaIndex].metadata[0].description;
return metadata.find((m) =>
m.metadata.find((o) => o.name === object.name),
)?.metadata[0]?.description;
},
},
Library: {
Expand Down
2 changes: 1 addition & 1 deletion federation-integration-testsuite-js/src/fixtures/reviews.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export const resolvers: GraphQLResolverMap<any> = {
},
Mutation: {
reviewProduct(_, { upc, body }) {
const id = `${Number(reviews[reviews.length - 1].id) + 1}`;
const id = `${Number(reviews[reviews.length - 1]!.id) + 1}`;
reviews.push({
id,
authorID: '1',
Expand Down
95 changes: 46 additions & 49 deletions federation-js/src/composition/compose.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ import {
stripTypeSystemDirectivesFromTypeDefs,
defaultRootOperationNameLookup,
getFederationMetadata,
CompositionResult
CompositionResult,
getOrSetRecord
} from './utils';
import {
ServiceDefinition,
Expand Down Expand Up @@ -162,16 +163,23 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {

for (const keyDirective of findDirectivesOnNode(definition, 'key')) {
if (
keyDirective.arguments &&
keyDirective.arguments?.[0] &&
isStringValueNode(keyDirective.arguments[0].value)
) {
// Initialize the entry for this type if necessary
keyDirectivesMap[typeName] = keyDirectivesMap[typeName] || {};
const serviceNameToKeyDirectives = getOrSetRecord(
keyDirectivesMap,
typeName,
{},
);
// Initialize the entry for this service if necessary
keyDirectivesMap[typeName][serviceName] =
keyDirectivesMap[typeName][serviceName] || [];
const keyDirectives = getOrSetRecord(
serviceNameToKeyDirectives,
serviceName,
[],
);
// Add @key metadata to the array
keyDirectivesMap[typeName][serviceName]!.push(
keyDirectives.push(
parseSelections(keyDirective.arguments[0].value.value),
);
}
Expand All @@ -185,13 +193,9 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
* 1. It was declared by a previous service, but this newer one takes precedence, or...
* 2. It was extended by a service before declared
*/
if (!typeToServiceMap[typeName]) {
typeToServiceMap[typeName] = {
extensionFieldsToOwningServiceMap: Object.create(null),
};
}

typeToServiceMap[typeName].owningService = serviceName;
getOrSetRecord(typeToServiceMap, typeName, {
extensionFieldsToOwningServiceMap: Object.create(null),
}).owningService = serviceName;

/**
* If this type already exists in the definitions map, push this definition to the array (newer defs
Expand All @@ -200,22 +204,18 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
*
* If not, create the definitions array and add it to the typeDefinitionsMap.
*/
if (typeDefinitionsMap[typeName]) {
const typeDefinitions = getOrSetRecord(typeDefinitionsMap, typeName, []);
if (typeDefinitions.length) {
const isValueType = typeNodesAreEquivalent(
typeDefinitionsMap[typeName][
typeDefinitionsMap[typeName].length - 1
],
typeDefinitions[typeDefinitions.length - 1]!,
definition,
);

if (isValueType) {
valueTypes.add(typeName);
}

typeDefinitionsMap[typeName].push({ ...definition, serviceName });
} else {
typeDefinitionsMap[typeName] = [{ ...definition, serviceName }];
}
typeDefinitions.push({ ...definition, serviceName });
} else if (isTypeExtensionNode(definition)) {
const typeName = definition.name.value;

Expand All @@ -237,9 +237,10 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
* and add the extensionFieldsToOwningServiceMap, but don't add a serviceName. That will be added once that service
* definition is processed.
*/
if (typeToServiceMap[typeName]) {
typeToServiceMap[typeName].extensionFieldsToOwningServiceMap = {
...typeToServiceMap[typeName].extensionFieldsToOwningServiceMap,
const serviceMap = typeToServiceMap[typeName];
if (serviceMap) {
serviceMap.extensionFieldsToOwningServiceMap = {
...serviceMap.extensionFieldsToOwningServiceMap,
...fields,
};
} else {
Expand All @@ -257,9 +258,10 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
serviceName,
);

if (typeToServiceMap[typeName]) {
typeToServiceMap[typeName].extensionFieldsToOwningServiceMap = {
...typeToServiceMap[typeName].extensionFieldsToOwningServiceMap,
const serviceMap = typeToServiceMap[typeName];
if (serviceMap) {
serviceMap.extensionFieldsToOwningServiceMap = {
...serviceMap.extensionFieldsToOwningServiceMap,
...values,
};
} else {
Expand All @@ -274,11 +276,10 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
* array (since a type can be extended by multiple services). If not, create the extensions array
* and add it to the typeExtensionsMap.
*/
if (typeExtensionsMap[typeName]) {
typeExtensionsMap[typeName].push({ ...definition, serviceName });
} else {
typeExtensionsMap[typeName] = [{ ...definition, serviceName }];
}
getOrSetRecord(typeExtensionsMap, typeName, []).push({
...definition,
serviceName,
});
} else if (definition.kind === Kind.DIRECTIVE_DEFINITION) {
const directiveName = definition.name.value;

Expand All @@ -299,15 +300,9 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
locations: executableLocations,
};

if (directiveDefinitionsMap[directiveName]) {
directiveDefinitionsMap[directiveName][
serviceName
] = definitionWithExecutableLocations;
} else {
directiveDefinitionsMap[directiveName] = {
[serviceName]: definitionWithExecutableLocations,
};
}
getOrSetRecord(directiveDefinitionsMap, directiveName, {})[
serviceName
] = definitionWithExecutableLocations;
}
}
}
Expand Down Expand Up @@ -393,14 +388,14 @@ export function buildSchemaFromDefinitionsAndExtensions({
return [
...rest,
{
...first,
...first!,
interfaces: Array.from(uniqueInterfaces.values()),
},
];

}),
...Object.values(directiveDefinitionsMap).map(
definitions => Object.values(definitions)[0],
definitions => Object.values(definitions)[0]!,
),
],
};
Expand Down Expand Up @@ -493,7 +488,7 @@ export function addFederationMetadataToSchemaNodes({

if (
providesDirective &&
providesDirective.arguments &&
providesDirective.arguments?.[0] &&
isStringValueNode(providesDirective.arguments[0].value)
) {
const fieldFederationMetadata: FederationField = {
Expand Down Expand Up @@ -543,7 +538,7 @@ export function addFederationMetadataToSchemaNodes({

if (
requiresDirective &&
requiresDirective.arguments &&
requiresDirective.arguments?.[0] &&
isStringValueNode(requiresDirective.arguments[0].value)
) {
const fieldFederationMetadata: FederationField = {
Expand Down Expand Up @@ -585,19 +580,21 @@ export function addFederationMetadataToSchemaNodes({
}

// add all definitions of a specific directive for validation later
for (const directiveName of Object.keys(directiveDefinitionsMap)) {
for (const [directiveName, directiveDefinitions] of Object.entries(
directiveDefinitionsMap,
)) {
const directive = schema.getDirective(directiveName);
if (!directive) continue;

const directiveFederationMetadata: FederationDirective = {
...getFederationMetadata(directive),
directiveDefinitions: directiveDefinitionsMap[directiveName],
}
directiveDefinitions,
};

directive.extensions = {
...directive.extensions,
federation: directiveFederationMetadata,
}
};
}
}

Expand Down
2 changes: 1 addition & 1 deletion federation-js/src/composition/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export interface ExternalFieldDefinition {
}

export interface ServiceNameToKeyDirectivesMap {
[serviceName: string]: ReadonlyArray<SelectionNode>[] | undefined;
[serviceName: string]: ReadonlyArray<SelectionNode>[];
}

export interface FederationType {
Expand Down
47 changes: 29 additions & 18 deletions federation-js/src/composition/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ export function hasMatchingFieldInDirectives({
directives
// for each key directive, get the fields arg
.map(keyDirective =>
keyDirective.arguments &&
keyDirective.arguments?.[0] &&
isStringValueNode(keyDirective.arguments[0].value)
? {
typeName: namedType.astNode!.name.value,
Expand Down Expand Up @@ -313,15 +313,12 @@ export function selectionIncludesField({
// if the field selection has a subselection, check each field recursively

// check to make sure the parent type contains the field
const typeIncludesField =
selectionName &&
Object.keys(selectionSetType.getFields()).includes(selectionName);
if (!selectionName || !typeIncludesField) continue;
if (!selectionName) continue;
const field = selectionSetType.getFields()[selectionName];
if (!field) continue;

// get the return type of the selection
const returnType = getNamedType(
selectionSetType.getFields()[selectionName].type,
);
const returnType = getNamedType(field.type);
if (!returnType || !isObjectType(returnType)) continue;
const subselections =
selection.selectionSet && selection.selectionSet.selections;
Expand Down Expand Up @@ -383,11 +380,11 @@ export function diffTypeNodes(
secondNode: TypeDefinitionNode | TypeExtensionNode | DirectiveDefinitionNode,
) {
const fieldsDiff: {
[fieldName: string]: string[];
[fieldName: string]: [string, ...string[]];
} = Object.create(null);

const inputValuesDiff: {
[inputName: string]: string[];
[inputName: string]: [string, ...string[]];
} = Object.create(null);

const unionTypesDiff: {
Expand All @@ -397,7 +394,7 @@ export function diffTypeNodes(
const locationsDiff: Set<string> = new Set();

const argumentsDiff: {
[argumentName: string]: string[];
[argumentName: string]: [string, ...string[]];
} = Object.create(null);

const document: DocumentNode = {
Expand All @@ -410,14 +407,14 @@ export function diffTypeNodes(

const type = print(node.type);

if (!fieldsDiff[fieldName]) {
const fieldTypes = fieldsDiff[fieldName];
if (!fieldTypes) {
fieldsDiff[fieldName] = [type];
return;
}

// If we've seen this field twice and the types are the same, remove this
// field from the diff result
const fieldTypes = fieldsDiff[fieldName];
if (fieldTypes[0] === type) {
delete fieldsDiff[fieldName];
} else {
Expand All @@ -434,14 +431,14 @@ export function diffTypeNodes(

const type = print(node.type);

if (!inputValuesDiff[fieldName]) {
const inputValueTypes = inputValuesDiff[fieldName];
if (!inputValueTypes) {
inputValuesDiff[fieldName] = [type];
return;
}

// If we've seen this input value twice and the types are the same,
// remove it from the diff result
const inputValueTypes = inputValuesDiff[fieldName];
if (inputValueTypes[0] === type) {
delete inputValuesDiff[fieldName];
} else {
Expand Down Expand Up @@ -484,13 +481,14 @@ export function diffTypeNodes(
node.arguments.forEach(argument => {
const argumentName = argument.name.value;
const printedType = print(argument.type);
if (argumentsDiff[argumentName]) {
if (printedType === argumentsDiff[argumentName][0]) {
const argumentTypes = argumentsDiff[argumentName];
if (argumentTypes) {
if (printedType === argumentTypes[0]) {
// If the existing entry is equal to printedType, it means there's no
// diff, so we can remove the entry from the diff object
delete argumentsDiff[argumentName];
} else {
argumentsDiff[argumentName].push(printedType);
argumentTypes.push(printedType);
}
} else {
argumentsDiff[argumentName] = [printedType];
Expand Down Expand Up @@ -661,3 +659,16 @@ export function getFederationMetadata(obj: any) {
else if (isDirective(obj)) return obj.extensions?.federation as FederationDirective | undefined;
else return obj.extensions?.federation as FederationField | undefined;
}

export function getOrSetRecord<V, D extends V>(
record: Record<string, V>,
key: string,
defaultValue: D,
): V {
const alreadyThere = record[key];
if (alreadyThere) {
return alreadyThere;
}
record[key] = defaultValue;
return defaultValue;
}
2 changes: 1 addition & 1 deletion federation-js/src/composition/validate/postComposition/executableDirectivesIdentical.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const executableDirectivesIdentical: PostCompositionValidator = ({
const shouldError = definitions.some(([, definition], index) => {
// Skip the non-comparison step
if (index === 0) return;
const [, previousDefinition] = definitions[index - 1];
const [, previousDefinition] = definitions[index - 1]!;
return !typeNodesAreEquivalent(definition, previousDefinition);
});

Expand Down

0 comments on commit 123303f

Please sign in to comment.