Skip to content

Commit

Permalink
feat: consider input and argument usage for breaking change detection (
Browse files Browse the repository at this point in the history
…#255)

Co-authored-by: Dustin Deus <deusdustin@gmail.com>
  • Loading branch information
jensneuse and StarpTech authored Nov 15, 2023
1 parent 55f2707 commit e10ac40
Show file tree
Hide file tree
Showing 36 changed files with 1,513 additions and 392 deletions.
8 changes: 5 additions & 3 deletions cli/src/commands/subgraph/commands/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,16 @@ export default (opts: BaseCommandOptions) => {

console.log(`\nChecking the proposed schema for subgraph ${pc.bold(name)}.`);

// No operations imply no clients and no subgraphs
// No operations usage stats mean the check was not performed against any live traffic
if (resp.operationUsageStats) {
if (resp.operationUsageStats?.totalOperations === 0) {
// Composition errors are still considered failures
if (resp.operationUsageStats.totalOperations === 0) {
// Composition errors are still considered failures, otherwise we can consider this a success
// because no operations were affected by the change
success = resp.compositionErrors.length === 0;
console.log(`No operations were affected by this schema change.`);
finalStatement = `This schema change didn't affect any operations from existing client traffic.`;
} else {
// Composition and breaking errors are considered failures because operations were affected by the change
success = resp.breakingChanges.length === 0 && resp.compositionErrors.length === 0;

console.log(
Expand Down
3 changes: 3 additions & 0 deletions connect/src/wg/cosmo/platform/v1/platform_pb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,9 @@ export class CheckSubgraphSchemaResponse extends Message<CheckSubgraphSchemaResp
compositionErrors: CompositionError[] = [];

/**
* Contains the operation usage stats for the operations that are impacted by the schema changes.
* Can be undefined when the schema changes are not inspectable by real traffic breaking change detection.
*
* @generated from field: wg.cosmo.platform.v1.CheckOperationUsageStats operationUsageStats = 5;
*/
operationUsageStats?: CheckOperationUsageStats;
Expand Down
31 changes: 18 additions & 13 deletions controlplane/src/core/bufservices/PlatformService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { OrganizationEventName, PlatformEventName } from '@wundergraph/cosmo-con
import { PlatformService } from '@wundergraph/cosmo-connect/dist/platform/v1/platform_connect';
import {
CheckFederatedGraphResponse,
CheckOperationUsageStats,
CheckSubgraphSchemaResponse,
CompositionError,
CreateAPIKeyResponse,
Expand Down Expand Up @@ -91,6 +90,7 @@ import {
InspectorOperationResult,
SchemaUsageTrafficInspector,
collectOperationUsageStats,
InspectorSchemaChange,
} from '../services/SchemaUsageTrafficInspector.js';
import Slack from '../services/Slack.js';
import {
Expand Down Expand Up @@ -598,6 +598,7 @@ export default function (opts: RouterOptions): Partial<ServiceImpl<typeof Platfo
};
}

let isInspectable = true;
const hasBreakingChanges = schemaChanges.breakingChanges.length > 0;

await schemaCheckRepo.createSchemaCheckChanges({
Expand Down Expand Up @@ -627,11 +628,17 @@ export default function (opts: RouterOptions): Partial<ServiceImpl<typeof Platfo
const inspectedOperations: InspectorOperationResult[] = [];
const compositionErrors: PlainMessage<CompositionError>[] = [];

// For operations checks we only consider breaking changes
const inspectorChanges = trafficInspector.schemaChangesToInspectorChanges(
schemaChanges.breakingChanges,
storedBreakingChanges,
);
let inspectorChanges: InspectorSchemaChange[] = [];
try {
// For operations checks we only consider breaking changes
// This method will throw if the schema changes cannot be converted to inspector changes
inspectorChanges = trafficInspector.schemaChangesToInspectorChanges(
schemaChanges.breakingChanges,
storedBreakingChanges,
);
} catch {
isInspectable = false;
}

for (const composition of result.compositions) {
const graphConfig = await fedGraphRepo.getConfig(composition.id);
Expand All @@ -652,13 +659,14 @@ export default function (opts: RouterOptions): Partial<ServiceImpl<typeof Platfo
}
}

// We don't collect operation usage when we have composition errors
if (composition.errors.length === 0 && inspectorChanges.inspectable && inspectorChanges.changes.length > 0) {
// We don't collect operation usage when we have composition errors or
// when we don't have any inspectable changes. That means any breaking change is really breaking
if (composition.errors.length === 0 && isInspectable && inspectorChanges.length > 0) {
if (graphConfig.trafficCheckDays <= 0) {
continue;
}

const result = await trafficInspector.inspect(inspectorChanges.changes, {
const result = await trafficInspector.inspect(inspectorChanges, {
daysToConsider: graphConfig.trafficCheckDays,
federatedGraphId: composition.id,
organizationId: authContext.organizationId,
Expand Down Expand Up @@ -687,9 +695,6 @@ export default function (opts: RouterOptions): Partial<ServiceImpl<typeof Platfo
hasBreakingChanges,
});

const operationUsageStats: PlainMessage<CheckOperationUsageStats> =
collectOperationUsageStats(inspectedOperations);

if (req.gitInfo && opts.githubApp) {
const githubRepo = new GitHubRepository(opts.db, opts.githubApp);
await githubRepo.createCommitCheck({
Expand All @@ -711,7 +716,7 @@ export default function (opts: RouterOptions): Partial<ServiceImpl<typeof Platfo
},
breakingChanges: schemaChanges.breakingChanges,
nonBreakingChanges: schemaChanges.nonBreakingChanges,
operationUsageStats,
operationUsageStats: isInspectable ? collectOperationUsageStats(inspectedOperations) : undefined,
compositionErrors,
};
});
Expand Down
29 changes: 19 additions & 10 deletions controlplane/src/core/composition/schemaCheck.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,24 @@ export interface GetDiffBetweenGraphsFailure {

export type GetDiffBetweenGraphsResult = GetDiffBetweenGraphsSuccess | GetDiffBetweenGraphsFailure;

export async function getSchemaDiff(oldSchemaSDL: GraphQLSchema, newSchemaSDL: GraphQLSchema): Promise<SchemaDiff[]> {
const changes = await diff(oldSchemaSDL, newSchemaSDL);
return changes.map((change) => {
return {
message: change.message,
changeType: change.type,
path: change.path ?? '',
isBreaking:
change.criticality.level === CriticalityLevel.Breaking ||
// We consider enum value changes as breaking changes because it is common to use enums in switch statements
// and if a value is removed or added, the switch statement will not be exhaustive anymore and might
// lead to unexpected behavior.
change.type === ChangeType.EnumValueRemoved ||
change.type === ChangeType.EnumValueAdded,
};
});
}

export async function getDiffBetweenGraphs(
oldSchemaSDL: string,
newSchemaSDL?: string,
Expand Down Expand Up @@ -56,16 +74,7 @@ export async function getDiffBetweenGraphs(
}
}

const changes: Change<ChangeType>[] = await diff(oldSchema, newSchema);

const schemaChanges: SchemaDiff[] = changes.map((change) => {
return {
message: change.message,
changeType: change.type,
path: change.path ?? '',
isBreaking: change.criticality.level === CriticalityLevel.Breaking,
};
});
const schemaChanges = await getSchemaDiff(oldSchema, newSchema);

const breakingChanges = schemaChanges.filter((change) => change.isBreaking);

Expand Down
2 changes: 0 additions & 2 deletions controlplane/src/core/services/AccessTokenAuthenticator.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import { EnumStatusCode } from '@wundergraph/cosmo-connect/dist/common/common_pb';
import { addDays } from 'date-fns';
import AuthUtils from '../auth-utils.js';
import { AuthenticationError } from '../errors/errors.js';
import { OrganizationRepository } from '../repositories/OrganizationRepository.js';
import { checkUserAccess } from '../util.js';
import { calLink } from './Authentication.js';

export type AccessTokenAuthContext = {
userId: string;
Expand Down
2 changes: 0 additions & 2 deletions controlplane/src/core/services/ApiKeyAuthenticator.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import { EnumStatusCode } from '@wundergraph/cosmo-connect/dist/common/common_pb';
import { addDays } from 'date-fns';
import { eq } from 'drizzle-orm';
import { PostgresJsDatabase } from 'drizzle-orm/postgres-js';
import * as schema from '../../db/schema.js';
import { AuthenticationError } from '../errors/errors.js';
import { OrganizationRepository } from '../repositories/OrganizationRepository.js';
import { calLink } from './Authentication.js';

export type ApiKeyAuthContext = {
organizationId: string;
Expand Down
3 changes: 0 additions & 3 deletions controlplane/src/core/services/Authentication.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { EnumStatusCode } from '@wundergraph/cosmo-connect/dist/common/common_pb';
import { addDays } from 'date-fns';
import { lru } from 'tiny-lru';
import { AuthContext } from '../../types/index.js';
import { AuthenticationError } from '../errors/errors.js';
Expand All @@ -13,8 +12,6 @@ import WebSessionAuthenticator from './WebSessionAuthenticator.js';
// The maximum time to cache the user auth context for the web session authentication.
const maxAuthCacheTtl = 30 * 1000; // 30 seconds

export const calLink = 'https://cal.com/stefan-avram-wundergraph/wundergraph-introduction';

export interface Authenticator {
authenticate(headers: Headers): Promise<AuthContext>;
authenticateRouter(headers: Headers): Promise<GraphKeyAuthContext>;
Expand Down
196 changes: 196 additions & 0 deletions controlplane/src/core/services/SchemaUsageTrafficInspector.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
import { describe, expect, test } from 'vitest';
import { buildSchema, GraphQLSchema } from 'graphql';
import { getSchemaDiff } from '../composition/schemaCheck.js';
import { InspectorSchemaChange, toInspectorChange } from './SchemaUsageTrafficInspector.js';

describe('Schema Change converter', (ctx) => {
describe('Arguments', (ctx) => {
test('Add a new required argument', async () => {
const a = buildSchema(/* GraphQL */ `
type Query {
a: String
}
`);
const b = buildSchema(/* GraphQL */ `
type Query {
a(b: Boolean!): String
}
`);

const changes = await getBreakingChanges(a, b);

expect(changes).toEqual<InspectorSchemaChange[]>([
{
isArgument: true,
path: ['a', 'b'],
schemaChangeId: '0',
typeName: 'Query',
},
]);

test('Add a new required argument nested', async () => {
const a = buildSchema(/* GraphQL */ `
type Rocket {
details: String
}
type Query {
a(b: Boolean!): Rocket
}
`);
const b = buildSchema(/* GraphQL */ `
type Rocket {
details(all: Boolean!): String
}
type Query {
a(b: Boolean!): Rocket
}
`);

const changes = await getBreakingChanges(a, b);

expect(changes).toEqual<InspectorSchemaChange[]>([
{
isArgument: true,
path: ['details', 'all'],
schemaChangeId: '0',
typeName: 'Rocket',
},
]);
});
});
});

describe('Input', (ctx) => {
test('Add a new required Input field', async () => {
const a = buildSchema(/* GraphQL */ `
input Foo {
a: String!
}
`);
const b = buildSchema(/* GraphQL */ `
input Foo {
a: String!
b: String!
}
`);

const changes = await getBreakingChanges(a, b);

expect(changes).toEqual<InspectorSchemaChange[]>([
{
fieldName: 'b',
isInput: true,
schemaChangeId: '0',
typeName: 'Foo',
},
]);
});

test('Change the type of an Input field', async () => {
const a = buildSchema(/* GraphQL */ `
input Foo {
a: String!
}
`);
const b = buildSchema(/* GraphQL */ `
input Foo {
a: Int!
}
`);

const changes = await getBreakingChanges(a, b);

expect(changes).toEqual<InspectorSchemaChange[]>([
{
fieldName: 'a',
isInput: true,
schemaChangeId: '0',
typeName: 'Foo',
},
]);
});
});

describe('Types', (ctx) => {
test('Type removed', async () => {
const a = buildSchema(/* GraphQL */ `
type Rocket {
details: String
}
type Query {
a(b: Boolean!): Rocket
}
`);
const b = buildSchema(/* GraphQL */ `
type Query {
a(b: Boolean!): String
}
`);

const changes = await getBreakingChanges(a, b);

expect(changes).toEqual<InspectorSchemaChange[]>([
{
schemaChangeId: '0',
typeName: 'Rocket',
},
{
fieldName: 'a',
schemaChangeId: '1',
typeName: 'Query',
},
]);
});
});

describe('Enums', (ctx) => {
test('Enum Value added', async () => {
const a = buildSchema(/* GraphQL */ `
type Query {
fieldA: String
}
enum enumA {
A
}
`);

const b = buildSchema(/* GraphQL */ `
type Query {
fieldA: String
}
enum enumA {
A
B
}
`);

const changes = await getBreakingChanges(a, b);

expect(changes).toEqual<InspectorSchemaChange[]>([
{
namedType: 'enumA',
schemaChangeId: '0',
},
]);
});
});
});

async function getBreakingChanges(a: GraphQLSchema, b: GraphQLSchema): Promise<InspectorSchemaChange[]> {
const changes = await getSchemaDiff(a, b);
return changes
.map((c, i) =>
toInspectorChange(
{
path: c.path!,
message: c.message,
changeType: c.changeType,
isBreaking: c.isBreaking,
},
i.toString(),
),
)
.filter((c) => c !== null) as InspectorSchemaChange[];
}
Loading

0 comments on commit e10ac40

Please sign in to comment.