Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add quick fixes to add types and fields #332

Merged
merged 3 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core-exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,4 @@ export { TransactionError } from './src/execution/transaction-error';
export { ConflictRetriesExhaustedError } from './src/execution/runtime-errors';
export { AuthContext } from './src/authorization/auth-basics';
export { ChangeSet, TextChange } from './src/model/change-set/change-set';
export { applyChangeSet, applyChanges } from './src/model/change-set/apply-changes';
export { applyChangeSet } from './src/model/change-set/apply-change-set';
5 changes: 4 additions & 1 deletion spec/model/change-set/apply-change-set.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { expect } from 'chai';
import { MessageLocation } from '../../../src/model';
import { applyChangeSet, InvalidChangeSetError } from '../../../src/model/change-set/apply-changes';
import {
applyChangeSet,
InvalidChangeSetError,
} from '../../../src/model/change-set/apply-change-set';
import { ChangeSet, TextChange } from '../../../src/model/change-set/change-set';
import { Project } from '../../../src/project/project';
import { ProjectSource } from '../../../src/project/source';
Expand Down
35 changes: 29 additions & 6 deletions spec/model/compatibility-check/check-model.spec.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import gql from 'graphql-tag';
import {
expectQuickFix,
expectSingleCompatibilityIssue,
expectToBeValid
expectToBeValid,
} from '../implementation/validation-utils';
import { runCheck } from './utils';
import { Project, ProjectSource } from '../../../core-exports';
import { print } from 'graphql';

describe('checkModel', () => {
describe('basics', () => {
Expand All @@ -24,22 +27,42 @@ describe('checkModel', () => {
});

it('rejects if a type is missing', () => {
const projectToCheck = new Project([
new ProjectSource(
// use the same name as in the baseline project to test the case where the quickfix appends it to the file
'baseline.graphql',
print(gql`
type WrongTypeName @rootEntity {
field: String
}
`),
),
]);
const result = runCheck(
gql`
type Test @rootEntity @modules(in: "module1") {
field: String @modules(all: true)
}
`,
gql`
type WrongTypeName @rootEntity {
field: String
}
`,
projectToCheck,
);
expectSingleCompatibilityIssue(
result,
'Type "Test" is missing (required by module "module1").',
);
expectQuickFix(
result,
'Add type "Test"',
`type WrongTypeName @rootEntity {
field: String
}

type Test @rootEntity {
field: String
}
`,
{ project: projectToCheck },
);
});

it('rejects if a type is of a completely wrong kind', () => {
Expand Down
41 changes: 40 additions & 1 deletion spec/model/compatibility-check/check-object-types.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import gql from 'graphql-tag';
import { expectSingleCompatibilityIssue } from '../implementation/validation-utils';
import { expectQuickFix, expectSingleCompatibilityIssue } from '../implementation/validation-utils';
import { runCheck } from './utils';

describe('checkModel', () => {
Expand All @@ -21,6 +21,45 @@ describe('checkModel', () => {
result,
'Field "Test.field" is missing (required by module "module1").',
);
expectQuickFix(
result,
'Add field "field"',
`type Test @rootEntity {
wrongFieldName: String
field: String
}`,
);
});

it('includes field directives in the quickfix', () => {
const result = runCheck(
gql`
type Test @rootEntity @modules(in: "module1") {
field: String
@calcMutations(operators: APPEND)
@key
@roles(read: ["a", "b"])
@modules(all: true)
}
`,
gql`
type Test @rootEntity {
wrongFieldName: String
}
`,
);
expectSingleCompatibilityIssue(
result,
'Field "Test.field" is missing (required by module "module1").',
);
expectQuickFix(
result,
'Add field "field"',
`type Test @rootEntity {
wrongFieldName: String
field: String @calcMutations(operators: APPEND) @key @roles(read: ["a", "b"])
}`,
);
});
});
});
17 changes: 9 additions & 8 deletions spec/model/compatibility-check/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ import { DocumentNode, print } from 'graphql';
import { ValidationResult } from '../../../src/model/validation/result';
import { Project } from '../../../src/project/project';
import { ProjectSource } from '../../../src/project/source';
import {
expectNoErrors,
expectToBeValid
} from '../implementation/validation-utils';
import { expectNoErrors, expectToBeValid } from '../implementation/validation-utils';

interface RunOptions {
allowWarningsAndInfosInProjectToCheck?: boolean;
Expand All @@ -14,12 +11,16 @@ interface RunOptions {

export function runCheck(
baselineDoc: DocumentNode,
docToCheck: DocumentNode,
docToCheck: DocumentNode | Project,
options: RunOptions = {},
): ValidationResult {
const projectToCheck = new Project({
sources: [new ProjectSource('to-check.graphql', print(docToCheck))],
});
// allow this to be a Project in case the caller needs this for applyChangeSet
const projectToCheck =
docToCheck instanceof Project
? docToCheck
: new Project({
sources: [new ProjectSource('to-check.graphql', print(docToCheck))],
});
if (options.allowWarningsAndInfosInProjectToCheck) {
expectNoErrors(projectToCheck);
} else {
Expand Down
25 changes: 22 additions & 3 deletions spec/model/implementation/validation-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,20 @@ export function expectSingleMessage(component: Validatable, errorPart: string, s
expect(message.severity).to.equal(severity);
}

export interface ExpectQuickFixOptions {
/**
* Optionally, the project that was validated
*
* Needed if the quick fix change set is not just a simple TextChange on a single file
*/
readonly project?: Project;
}

export function expectQuickFix(
component: Validatable,
expectedDescription: string,
expectedBody: string,
{ project }: ExpectQuickFixOptions = {},
) {
const result = validate(component);
const quickfixes = result.messages.flatMap((m) => m.quickFixes);
Expand All @@ -76,10 +86,19 @@ export function expectQuickFix(
const quickfix = matchingQuickfixes[0];
const changeSet = quickfix.getChangeSet();
expect(changeSet.changes).not.to.be.empty;
// dirty hack that works because our quickfix tests only use one file
const project = new Project([changeSet.changes[0].location.source]);

// in most simple test cases, we can recreate the project from the quick fix
if (!project) {
expect(changeSet.appendChanges).to.be.empty;
const source = changeSet.textChanges[0].source;
for (const change of changeSet.textChanges) {
expect(change.source).to.equal(source);
}
project = new Project([source]);
}

const newProject = applyChangeSet(project, changeSet);
expectToBeValid(newProject); // expect the fix to acutally fix the issues
expectToBeValid(newProject); // expect the fix to actually fix the issues
expect(newProject.sources).to.have.a.lengthOf(1);
expect(newProject.sources[0].body).to.equal(expectedBody);
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,40 @@ export class InvalidChangeSetError extends Error {
}

export function applyChangeSet(project: Project, changeSet: ChangeSet): Project {
const newSources = project.sources.map((source) => {
const changes = changeSet.changes.filter((c) => c.source.name === source.name);
return applyChanges(source, changes);
const changedSources = project.sources.map((source) => {
const textChanges = changeSet.textChanges.filter((c) => c.source.name === source.name);
const appendChanges = changeSet.appendChanges.filter((c) => c.sourceName === source.name);
let newText = applyChanges(source, textChanges);
for (const change of appendChanges) {
newText += (newText.length ? '\n\n' : '') + change.text;
}
if (newText === source.body) {
return source;
} else {
return new ProjectSource(source.name, newText, source.filePath);
}
});
const existingSourceNames = new Set(project.sources.map((s) => s.name));
const appendSourceNames = new Set(changeSet.appendChanges.map((c) => c.sourceName));
const newSourceNames = [...appendSourceNames].filter((name) => !existingSourceNames.has(name));
const newSources = [...newSourceNames].map((sourceName) => {
const appendChanges = changeSet.appendChanges.filter((c) => c.sourceName === sourceName);
let newText = '';
for (const change of appendChanges) {
newText += (newText.length ? '\n\n' : '') + change.text;
}
return new ProjectSource(sourceName, newText);
});

return new Project({
...project,
sources: newSources,
sources: [...changedSources, ...newSources],
});
}

export function applyChanges(
source: ProjectSource,
changes: ReadonlyArray<TextChange>,
): ProjectSource {
function applyChanges(source: ProjectSource, changes: ReadonlyArray<TextChange>): string {
if (!changes.length) {
return source;
return source.body;
}

const sortedChanges = [...changes].sort(
Expand Down Expand Up @@ -55,7 +72,7 @@ export function applyChanges(
}
lastChange = change;
}
return new ProjectSource(source.name, output, source.filePath);
return output;
}

function formatChangeLocation(change: TextChange | undefined): string {
Expand Down
35 changes: 32 additions & 3 deletions src/model/change-set/change-set.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
import { Location } from 'graphql';
import { Project } from '../../project/project';
import { ProjectSource } from '../../project/source';
import { LocationLike, MessageLocation } from '../validation/location';
import { MessageLocation } from '../validation/location';

export type Change = TextChange | AppendChange;

/**
* A set of changes to one or multiple project sources
*/
export class ChangeSet {
constructor(readonly changes: ReadonlyArray<TextChange>) {}
readonly textChanges: ReadonlyArray<TextChange>;
readonly appendChanges: ReadonlyArray<AppendChange>;

constructor(readonly changes: ReadonlyArray<Change>) {
this.textChanges = changes.filter((c) => c instanceof TextChange);
this.appendChanges = changes.filter((c) => c instanceof AppendChange);
}
}

/**
* A change that either deletes a span in an existing source or replaces that span with new text
*/
export class TextChange {
readonly source: ProjectSource;

Expand Down Expand Up @@ -37,3 +47,22 @@ export class TextChange {
this.source = this.location.source;
}
}

/**
* A change that creates a new source with a given name or appends text to an existing source
*
* If there are multiple AppendToNewSource instances with the same sourceName in a changeset, the content of all of them will be appended to the source, separated by two newlines.
*/
export class AppendChange {
constructor(
/**
* The name of the new source
*/
readonly sourceName: string,

/**
* The text for the new source
*/
readonly text: string,
) {}
}
41 changes: 40 additions & 1 deletion src/model/compatibility-check/check-model.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { Kind, ObjectTypeDefinitionNode, print, TypeDefinitionNode } from 'graphql';
import { MODULES_DIRECTIVE } from '../../schema/constants';
import { AppendChange, ChangeSet } from '../change-set/change-set';
import { Model } from '../implementation';
import { ValidationResult, ValidationMessage, ValidationContext } from '../validation';
import { QuickFix, ValidationContext, ValidationMessage, ValidationResult } from '../validation';
import { checkType } from './check-type';
import { getRequiredBySuffix } from './describe-module-specification';

Expand All @@ -12,10 +15,46 @@ export function checkModel(modelToCheck: Model, baselineModel: Model): Validatio
for (const baselineType of baselineModel.types) {
const matchingType = modelToCheck.getType(baselineType.name);
if (!matchingType) {
const quickFixes: QuickFix[] = [];
if (baselineType.astNode) {
let cleanedAstNode: TypeDefinitionNode = {
...baselineType.astNode,
directives: (baselineType.astNode.directives ?? []).filter(
(d) => d.name.value !== MODULES_DIRECTIVE,
),
};
if (
baselineType.astNode.kind === Kind.OBJECT_TYPE_DEFINITION &&
cleanedAstNode.kind === Kind.OBJECT_TYPE_DEFINITION
) {
cleanedAstNode = {
...cleanedAstNode,
fields: baselineType.astNode.fields?.map((fieldDef) => ({
...fieldDef,
directives: (fieldDef.directives ?? []).filter(
(d) => d.name.value !== MODULES_DIRECTIVE,
),
})),
};
}
const sourceName = baselineType.astNode.loc?.source.name ?? 'new.graphqls';

quickFixes.push(
new QuickFix({
description: `Add type "${baselineType.name}"`,
isPreferred: true,
changeSet: new ChangeSet([
new AppendChange(sourceName, print(cleanedAstNode) + '\n'),
]),
}),
);
}

context.addMessage(
ValidationMessage.compatibilityIssue(
`Type "${baselineType.name}" is missing${getRequiredBySuffix(baselineType)}.`,
undefined,
{ quickFixes },
),
);
continue;
Expand Down
Loading
Loading