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

fix(graphql-transformer-common): improve generated graphql pluralization #7258

Merged
merged 1 commit into from
Jun 21, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ const path = require('path');
const { RelationalDBSchemaTransformer } = require('graphql-relational-schema-transformer');
const { RelationalDBTemplateGenerator, AuroraServerlessMySQLDatabaseReader } = require('graphql-relational-schema-transformer');
const { mergeTypeDefs } = require('@graphql-tools/merge');
const { ResourceDoesNotExistError, exitOnNextTick } = require('amplify-cli-core');

const { FeatureFlags, ResourceDoesNotExistError, exitOnNextTick } = require('amplify-cli-core');
const subcommand = 'add-graphql-datasource';
const categories = 'categories';
const category = 'api';
Expand Down Expand Up @@ -143,7 +142,11 @@ module.exports = {
context[rdsResourceName] = resourceName;
context[rdsDatasource] = datasource;
let template = templateGenerator.createTemplate(context);
template = templateGenerator.addRelationalResolvers(template, resolversDir);
template = templateGenerator.addRelationalResolvers(
template,
resolversDir,
FeatureFlags.getBoolean('graphqltransformer.improvePluralization'),
);
const cfn = templateGenerator.printCloudformationTemplate(template);

/**
Expand Down
8 changes: 7 additions & 1 deletion packages/amplify-cli-core/src/feature-flags/featureFlags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,12 @@ export class FeatureFlags {
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
},
{
name: 'improvePluralization',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
Comment on lines +519 to +522
Copy link
Contributor

@lawmicha lawmicha Oct 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set the flag to true for new projects, they immediately won't work with the existing iOS library. This is a breaking change, right?
before:

  • type Wish generates listWishs
  • type Wishes generates listWishess

after:

  • type Wish generates listWishes
  • type Wishes generates listWishes

How do new projects work with existing iOS library? (investigating in #8350) Is it because iOS library has logic coupled to how the name is generated, and shoudn't have in the first place? Should we have feature flags for the libraries as well?

},
{
name: 'validateTypeNameReservedWords',
type: 'boolean',
Expand Down Expand Up @@ -619,7 +625,7 @@ export class FeatureFlags {
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
}
},
]);

this.registerFlag('appSync', [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ export const expected_result_query6 = {

export const query7 = `
## 7. See all employees hired recently:
#Having '@key(name: "newHire", fields: ["newHire", "id"])' on the 'Employee' model allows one to query by whether an employee has been hired recently.
#Having '@key(name: "newHire", fields: ["newHire", "id"])' on the 'Employee' model allows one to query by whether an employee has been hired recently.

query employeesNewHire {
employeesNewHire(newHire: "true") {
Expand Down Expand Up @@ -685,8 +685,8 @@ export const query16 = `
## 16. Get total product inventory:
#How this would be done depends on the use case. If one just wants a list of all inventories in all warehouses, one could just run a list inventories on the Inventory model:

query listInventorys {
listInventorys {
query listInventories {
listInventories {
items {
productID
warehouseID
Expand All @@ -696,7 +696,7 @@ query listInventorys {
}`;
export const expected_result_query16 = {
data: {
listInventorys: {
listInventories: {
items: [
{
productID: 'yeezyboost',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,17 @@ import { SearchableModelTransformer } from '../';
import { ModelTransformer } from '@aws-amplify/graphql-model-transformer';
import { anything, countResources, expect as cdkExpect, haveResource } from '@aws-cdk/assert';
import { parse } from 'graphql';
const featureFlags = {
getBoolean: jest.fn().mockImplementation((name, defaultValue) => {
if (name === 'improvePluralization') {
return true;
}
return;
}),
getNumber: jest.fn(),
getObject: jest.fn(),
getString: jest.fn(),
};

test('Test SearchableModelTransformer validation happy case', () => {
const validSchema = `
Expand All @@ -15,6 +26,7 @@ test('Test SearchableModelTransformer validation happy case', () => {
`;
const transformer = new GraphQLTransform({
transformers: [new ModelTransformer(), new SearchableModelTransformer()],
featureFlags,
});
const out = transformer.transform(validSchema);
expect(out).toBeDefined();
Expand All @@ -33,6 +45,7 @@ test('Test SearchableModelTransformer vtl', () => {
`;
const transformer = new GraphQLTransform({
transformers: [new ModelTransformer(), new SearchableModelTransformer()],
featureFlags,
});

const out = transformer.transform(validSchema);
Expand All @@ -50,6 +63,7 @@ test('Test SearchableModelTransformer with query overrides', () => {
`;
const transformer = new GraphQLTransform({
transformers: [new ModelTransformer(), new SearchableModelTransformer()],
featureFlags,
});
const out = transformer.transform(validSchema);
expect(out).toBeDefined();
Expand All @@ -67,6 +81,7 @@ test('Test SearchableModelTransformer with only create mutations', () => {
`;
const transformer = new GraphQLTransform({
transformers: [new ModelTransformer(), new SearchableModelTransformer()],
featureFlags,
});
const out = transformer.transform(validSchema);
expect(out).toBeDefined();
Expand All @@ -90,6 +105,7 @@ test('Test SearchableModelTransformer with multiple model searchable directives'
`;
const transformer = new GraphQLTransform({
transformers: [new ModelTransformer(), new SearchableModelTransformer()],
featureFlags,
});
const out = transformer.transform(validSchema);
expect(out).toBeDefined();
Expand All @@ -108,6 +124,7 @@ test('Test SearchableModelTransformer with sort fields', () => {
`;
const transformer = new GraphQLTransform({
transformers: [new ModelTransformer(), new SearchableModelTransformer()],
featureFlags,
});
const out = transformer.transform(validSchema);
expect(out).toBeDefined();
Expand Down Expand Up @@ -137,6 +154,7 @@ test('it generates expected resources', () => {
`;
const transformer = new GraphQLTransform({
transformers: [new ModelTransformer(), new SearchableModelTransformer()],
featureFlags,
});
const out = transformer.transform(validSchema);
expect(out).toBeDefined();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ export class SearchableModelTransformer extends TransformerPluginBase {
searchFieldNameOverride = directiveArguments.queries.search;
}
}
const fieldName = searchFieldNameOverride ? searchFieldNameOverride : graphqlName(`search${plurality(toUpper(definition.name.value))}`);
const fieldName = searchFieldNameOverride
? searchFieldNameOverride
: graphqlName(`search${plurality(toUpper(definition.name.value), ctx.featureFlags.getBoolean('improvePluralization'))}`);
this.searchableObjectTypeDefinitions.push({
node: definition,
fieldName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export interface GraphQLTransformOptions {
readonly authConfig?: AppSyncAuthConfiguration;
readonly buildParameters?: Record<string, any>;
readonly stacks?: Record<string, Template>;
readonly featuerFlags?: FeatureFlagProvider;
readonly featureFlags?: FeatureFlagProvider;
}
export type StackMapping = { [resourceId: string]: string };
export class GraphQLTransform {
Expand Down Expand Up @@ -110,7 +110,7 @@ export class GraphQLTransform {
this.seenTransformations = {};
const parsedDocument = parse(schema);
this.app = new App();
const context = new TransformerContext(this.app, parsedDocument, this.stackMappingOverrides, this.options.featuerFlags);
const context = new TransformerContext(this.app, parsedDocument, this.stackMappingOverrides, this.options.featureFlags);
const validDirectiveNameMap = this.transformers.reduce(
(acc: any, t: TransformerPluginProvider) => ({ ...acc, [t.directive.name.value]: true }),
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,20 @@ export class TransformerContext implements TransformerContextProvider {
public readonly resourceHelper: TransformerResourceHelper;
public readonly featureFlags: FeatureFlagProvider;
public _api?: GraphQLAPIProvider;
constructor(app: App, public readonly inputDocument: DocumentNode, stackMapping: Record<string, string>, featuerFlags?: FeatureFlagProvider) {
constructor(
app: App,
public readonly inputDocument: DocumentNode,
stackMapping: Record<string, string>,
featureFlags?: FeatureFlagProvider,
) {
this.output = new TransformerOutput(inputDocument);
this.resolvers = new ResolverManager();
this.dataSources = new TransformerDataSourceManager();
this.providerRegistry = new TransformerContextProviderRegistry();
const stackManager = new StackManager(app, stackMapping);
this.stackManager = stackManager
const stackManager = new StackManager(app, stackMapping);
this.stackManager = stackManager;
this.resourceHelper = new TransformerResourceHelper(stackManager);
this.featureFlags = featuerFlags ?? new NoopFeatureFlagProvider();
this.featureFlags = featureFlags ?? new NoopFeatureFlagProvider();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ async function _buildProject(opts: ProjectOptions<TransformerFactoryArgs>) {
authConfig: opts.authConfig,
buildParameters: opts.buildParameters,
stacks: opts.projectConfig.stacks || {},
featuerFlags: new AmplifyCLIFeatureFlagAdapter(),
featureFlags: new AmplifyCLIFeatureFlagAdapter(),
});
return transform.transform(userProjectConfig.schema.toString());
}
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ test(`Test getSalary w/ Admin group protection not authorized`, async () => {
expect((req2.errors[0] as any).errorType).toEqual('Unauthorized');
});

test(`Test listSalarys w/ Admin group protection authorized`, async () => {
test(`Test listSalaries w/ Admin group protection authorized`, async () => {
const req = await GRAPHQL_CLIENT_1.query(`
mutation {
createSalary(input: { wage: 101 }) {
Expand All @@ -806,20 +806,20 @@ test(`Test listSalarys w/ Admin group protection authorized`, async () => {
expect(req.data.createSalary.wage).toEqual(101);
const req2 = await GRAPHQL_CLIENT_1.query(`
query {
listSalarys(filter: { wage: { eq: 101 }}) {
listSalaries(filter: { wage: { eq: 101 }}) {
items {
id
wage
}
}
}
`);
expect(req2.data.listSalarys.items.length).toEqual(1);
expect(req2.data.listSalarys.items[0].id).toEqual(req.data.createSalary.id);
expect(req2.data.listSalarys.items[0].wage).toEqual(101);
expect(req2.data.listSalaries.items.length).toEqual(1);
expect(req2.data.listSalaries.items[0].id).toEqual(req.data.createSalary.id);
expect(req2.data.listSalaries.items[0].wage).toEqual(101);
});

test(`Test listSalarys w/ Admin group protection not authorized`, async () => {
test(`Test listSalaries w/ Admin group protection not authorized`, async () => {
const req = await GRAPHQL_CLIENT_1.query(`
mutation {
createSalary(input: { wage: 102 }) {
Expand All @@ -833,15 +833,15 @@ test(`Test listSalarys w/ Admin group protection not authorized`, async () => {
expect(req.data.createSalary.wage).toEqual(102);
const req2 = await GRAPHQL_CLIENT_2.query(`
query {
listSalarys(filter: { wage: { eq: 102 }}) {
listSalaries(filter: { wage: { eq: 102 }}) {
items {
id
wage
}
}
}
`);
expect(req2.data.listSalarys.items).toEqual([]);
expect(req2.data.listSalaries.items).toEqual([]);
});

/**
Expand Down Expand Up @@ -2374,7 +2374,7 @@ test(`Test createTestIdentity as admin.`, async () => {

const listResponse = await GRAPHQL_CLIENT_3.query(
`query {
listTestIdentitys(filter: { title: { eq: "Test title update" } }, limit: 100) {
listTestIdentities(filter: { title: { eq: "Test title update" } }, limit: 100) {
items {
id
title
Expand All @@ -2384,7 +2384,7 @@ test(`Test createTestIdentity as admin.`, async () => {
}`,
{},
);
const relevantPost = listResponse.data.listTestIdentitys.items.find(p => p.id === getReq.data.getTestIdentity.id);
const relevantPost = listResponse.data.listTestIdentities.items.find(p => p.id === getReq.data.getTestIdentity.id);
logDebug(JSON.stringify(listResponse, null, 4));
expect(relevantPost).toBeTruthy();
expect(relevantPost.title).toEqual('Test title update');
Expand Down
8 changes: 6 additions & 2 deletions packages/graphql-auth-transformer/src/ModelAuthTransformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ export class ModelAuthTransformer extends Transformer {
this.propagateAuthDirectivesToNestedTypes(def, rules, ctx);

// Retrieve the configuration options for the related @model directive
const modelConfiguration = new ModelDirectiveConfiguration(modelDirective, def);
const modelConfiguration = new ModelDirectiveConfiguration(modelDirective, def, ctx.featureFlags.getBoolean('improvePluralization'));

// Get the directives we need to add to the GraphQL nodes
const directives = this.getDirectivesForRules(rules, rules.length === 0 ? this.shouldAddDefaultAuthDirective() : false);
Expand Down Expand Up @@ -516,7 +516,11 @@ Static group authorization should perform as expected.`,
const isDeleteRule = isOpRule('delete');

// Retrieve the configuration options for the related @model directive
const modelConfiguration = new ModelDirectiveConfiguration(modelDirective, parent);
const modelConfiguration = new ModelDirectiveConfiguration(
modelDirective,
parent,
ctx.featureFlags.getBoolean('improvePluralization'),
);
// The field handler adds the read rule on the object
const readRules = rules.filter((rule: AuthRule) => isReadRule(rule));
this.protectReadForField(ctx, parent, definition, readRules, modelConfiguration);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,14 @@ type ModelDirectiveOperation = {
export class ModelDirectiveConfiguration {
map: Map<ModelDirectiveOperationType, ModelDirectiveOperation> = new Map();

constructor(directive: DirectiveNode, def: ObjectTypeDefinitionNode) {
constructor(directive: DirectiveNode, def: ObjectTypeDefinitionNode, improvePluralization: boolean) {
const typeName = def.name.value;
const directiveArguments: ModelDirectiveArgs = getDirectiveArguments(directive);

const makeName = (operation: ModelDirectiveOperationType, nameOverride?: string, isList: boolean = false) =>
nameOverride ? nameOverride : graphqlName(operation + (isList ? plurality(toUpper(typeName)) : toUpper(typeName)));
nameOverride
? nameOverride
: graphqlName(operation + (isList ? plurality(toUpper(typeName), improvePluralization) : toUpper(typeName)));

let shouldHaveCreate = true;
let shouldHaveUpdate = true;
Expand Down
Loading