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

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

merged 1 commit into from
Jun 21, 2021

Conversation

johnpc
Copy link
Contributor

@johnpc johnpc commented May 5, 2021

Description of changes

This commit adds the pluralize npm library instead of blindly appending an "s" to acheive
pluralization. For example, a model named "Match" should pluralize to "Matches", but before this
commit Amplify would pluralize it to "Matchs"

Issue #, if available

This PR fixes #4224

It's worth noting that another pull request (#7030) was created to solve the same issue. However, multiple comments were left with suggestions to fix, and the author of the PR hasn't pushed out an update for a few weeks.

Additionally, that PR as written only solves the problem for models that end in an "s" or are otherwise already plural. It did not solve the issue in my case (the "Match" model being pluralized to "Matchs").

It's worth noting that pr #7030 also updated .circleci/config.yml. I couldn't figure out why that change was necessary. If it is, please let me know and I'll add it here.

Description of how you validated changes

  • Updated unit test snapshot
  • Used amplify-devcli with previously incorrectly pluralized model names (Tomatoes and Match)

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

image

@johnpc johnpc requested a review from a team as a code owner May 5, 2021 05:05
@cjihrig
Copy link
Contributor

cjihrig commented May 5, 2021

Thanks for the PR @johnpc. As mentioned in #7030 and #4224, this work needs to be behind a feature flag before we can consider merging it.

@johnpc
Copy link
Contributor Author

johnpc commented May 5, 2021

Thanks for the feedback! I updated the pull request to put this change behind a feature flag. Unfortunately, since the plurality util is widely used, I had to add mocking across several test files. Please advise if there is a better way to accomplish it.

@SwaySway
Copy link
Contributor

SwaySway commented May 6, 2021

@johnpc You can add the FeatureFlag when creating an instance of GraphQL Transform. Example below:

const transformer = new GraphQLTransform({
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
featureFlags: {
getBoolean: (featureName: string, defaultValue: boolean) => {
if (featureName === 'secondaryKeyAsGSI') return false;
return defaultValue || false;
},
} as unknown as FeatureFlagProvider
});

@johnpc
Copy link
Contributor Author

johnpc commented May 6, 2021

does that apply to all the test files where I added the mock? Some of these files don't explicitly instantiate a GraphQLTransform instance

@codecov-commenter
Copy link

Codecov Report

Merging #7258 (0843e8d) into master (3dbb3bf) will increase coverage by 0.02%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7258      +/-   ##
==========================================
+ Coverage   52.72%   52.74%   +0.02%     
==========================================
  Files         513      513              
  Lines       25914    25917       +3     
  Branches     5058     5059       +1     
==========================================
+ Hits        13662    13669       +7     
+ Misses      11286    11282       -4     
  Partials      966      966              
Impacted Files Coverage Δ
...amplify-cli-core/src/feature-flags/featureFlags.ts 83.10% <ø> (ø)
...rc/graphql-transformer/transform-graphql-schema.ts 15.05% <ø> (ø)
...ages/graphql-dynamodb-transformer/src/resources.ts 71.42% <0.00%> (ø)
...arch-transformer/src/SearchableModelTransformer.ts 95.23% <ø> (ø)
...graphql-elasticsearch-transformer/src/resources.ts 92.59% <0.00%> (ø)
...-transformer/src/graphql-searchable-transformer.ts 97.02% <100.00%> (+0.02%) ⬆️
...aphql-auth-transformer/src/ModelAuthTransformer.ts 89.10% <100.00%> (+0.43%) ⬆️
...uth-transformer/src/ModelDirectiveConfiguration.ts 74.73% <100.00%> (+0.54%) ⬆️
...namodb-transformer/src/DynamoDBModelTransformer.ts 92.52% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3dbb3bf...0843e8d. Read the comment docs.

This commit adds the `pluralize` npm library instead of blindly appending an "s" to acheive
pluralization. For example, a model named "Match" should pluralize to "Matches", but before this
commit Amplify would pluralize it to "Matchs"

fix #4224
@edwardfoyle edwardfoyle merged commit fc3ad0d into aws-amplify:master Jun 21, 2021
@edwardfoyle
Copy link
Contributor

Reverting this PR as it appears to be causing a couple of e2e tests to fail: https://app.circleci.com/pipelines/github/aws-amplify/amplify-cli/1120/workflows/28f71966-47f4-476a-98da-a368a4bd69c3

edwardfoyle added a commit that referenced this pull request Jun 22, 2021
edwardfoyle added a commit that referenced this pull request Jun 22, 2021
@johnpc johnpc mentioned this pull request Jun 23, 2021
3 tasks
@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Jun 24, 2021
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v5.0.2 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v5.0.2.

@github-actions
Copy link

👋 Hi, this pull request was referenced in the v5.1.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v5.1.0.

cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
…ion (aws-amplify#7258)

This commit adds the `pluralize` npm library instead of blindly appending an "s" to acheive
pluralization. For example, a model named "Match" should pluralize to "Matches", but before this
commit Amplify would pluralize it to "Matchs"

fix aws-amplify#4224
cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
@lawmicha lawmicha mentioned this pull request Oct 3, 2021
4 tasks
Comment on lines +519 to +522
name: 'improvePluralization',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModelTransformer pluralization is incorrect for plural types
7 participants