Feature: Use improved pluralization #255

marcvberg · 2021-10-07T21:41:54Z

Description of changes

Use feature flag to determine whether to set pluralName or listPluralName/syncPluralName

Issue #, if available

Description of how you validated changes

Unit testing

Manually tested with JS, Android, and iOS applications. Manual tests involved creating Todos through the console as well as through the app, and observing that the data was still available while offline

Checklist

PR description included
yarn test passes
Tests are changed or added

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

...psync-modelgen-plugin/src/__tests__/visitors/__snapshots__/appsync-java-visitor.test.ts.snap

codecov-commenter · 2021-10-08T19:31:56Z

Codecov Report

Merging #255 (65e5bfa) into main (181480d) will increase coverage by 0.01%.
The diff coverage is 94.73%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   85.69%   85.71%   +0.01%     
==========================================
  Files         148      148              
  Lines        7380     7397      +17     
  Branches     1962     1973      +11     
==========================================
+ Hits         6324     6340      +16     
- Misses        959      960       +1     
  Partials       97       97

Impacted Files	Coverage Δ
...nc-modelgen-plugin/src/visitors/appsync-visitor.ts	`96.26% <ø> (ø)`
...elgen-plugin/src/visitors/appsync-swift-visitor.ts	`96.75% <80.00%> (-0.22%)`	⬇️
packages/amplify-codegen/src/commands/models.js	`87.40% <100.00%> (+0.09%)`	⬆️
...delgen-plugin/src/visitors/appsync-java-visitor.ts	`94.87% <100.00%> (+0.06%)`	⬆️
...ugin/src/visitors/appsync-json-metadata-visitor.ts	`96.07% <100.00%> (+0.20%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

AaronZyLee

The codegen logic LGTM. Still I have some concerns on each library change, which need further confirmation with library side.

JS(meta): the new attributes are appended instead of replacing the old one so it should not break customers
iOS and Android: the original plural field is replaced by two new fields, need library side release first
Flutter: we do not receive ticket from flutter and it usually depends on iOS and Android team, not sure if this should be included in this PR. The old plural field is also replaced and it might have impact

Also need a decision on the default feature flag value

manueliglesias

LGTM

JS team is OK with this change

dcipoletti · 2021-10-08T21:18:03Z

@manueliglesias We advise to hold off on rolling this out. Once the open CLI concerns are resolved and remedied, we will review and proceed.

lawmicha · 2021-10-14T19:07:36Z

Just a reminder, that codegen PR can be safely merged when the library changes are in for iOS and Android. Please track

iOS PR has been merged fix: Support new list name pluralization amplify-swift#1451
Android PR has been merged fix(api): replace pluralName with listPluralName & syncPluralName amplify-android#1523

There are flutter implications as well. Codeegn changes should be:

No changes to the Flutter dart files (no impact) - do not generate syncPluralName and listPluralName until later
Or generate all 3. add syncPluralName, listPluralName, and pluralName. Need to confirm with Flutter that this works
Or Flutter should take on the iOS and Android changes and any other necessary changes to support the new fields, then codegen can be safely merged

lawmicha · 2021-10-18T15:05:40Z

Are there any instructions you could provide to allow the library teams to run this change and generate the model files to make sure it works with the library before merging in the library changes? ie. install development version of the CLI which contains this codegen change so we can run amplify codegen models

HuiSF · 2021-10-21T16:47:53Z

Just a reminder, that codegen PR can be safely merged when the library changes are in for iOS and Android. Please track

iOS PR has been merged fix: Support new list name pluralization amplify-ios#1451

Android PR has been merged fix(api): replace pluralName with listPluralName & syncPluralName amplify-android#1523

There are flutter implications as well. Codeegn changes should be:

No changes to the Flutter dart files (no impact) - do not generate syncPluralName and listPluralName until later

Or generate all 3. add syncPluralName, listPluralName, and pluralName. Need to confirm with Flutter that this works

Or Flutter should take on the iOS and Android changes and any other necessary changes to support the new fields, then codegen can be safely merged

For amplify-flutter, can go with option 2. Without any changes to be made into amplify-flutter, only the pluralName field will be used, which should still work as native libraries are backwards compatible.

AaronZyLee · 2021-10-21T17:26:15Z

Are there any instructions you could provide to allow the library teams to run this change and generate the model files to make sure it works with the library before merging in the library changes? ie. install development version of the CLI which contains this codegen change so we can run amplify codegen models

You can follow the instruction here to pull this PR and run yarn setup-dev to build your local amplify-dev for testing. More info

AaronZyLee · 2021-10-21T17:29:06Z

packages/appsync-modelgen-plugin/src/visitors/appsync-dart-visitor.ts

@@ -610,13 +611,18 @@ export class AppSyncModelDartVisitor<
  }

  protected generateSchemaField(model: CodeGenModel, declarationBlock: DartDeclarationBlock): void {
+    const pluralFields = this.config.improvePluralization ? `modelSchemaDefinition.listPluralName = "${


As mentioned in the comment, Flutter need option 1 for the change (reverting flutter changes)

Please inform once this required change is complete, I will follow up and test. Thanks!

Correction: amplify-flutter should take the option 1, the dart-visitor should not make any changes.

@AaronZyLee removed

lawmicha · 2021-10-22T19:00:44Z

Are there any instructions you could provide to allow the library teams to run this change and generate the model files to make sure it works with the library before merging in the library changes? ie. install development version of the CLI which contains this codegen change so we can run amplify codegen models

You can follow the instruction here to pull this PR and run yarn setup-dev to build your local amplify-dev for testing. More info

thanks! i got this working with the following:

git remote add marcvberg https://github.com/marcvberg/amplify-codegen
git fetch marcvberg
git checkout -b improved-pluralization marcvberg/improved-pluralization
yarn setup-dev
// back over in the amplify project, run codegen model command to generate new models
cd <amplify project>
amplify-dev codegen models

lawmicha · 2021-11-03T21:28:19Z

Reminder this PR needs to be updated to remove the dart files changes related to Flutter (@HuiSF's comment #255 (comment))

lazpavel · 2021-11-18T17:26:14Z

packages/appsync-modelgen-plugin/src/visitors/appsync-json-metadata-visitor.ts

+    const pluralAttributes = this.config.improvePluralization ?
+      {
+        listPluralName: plurality(model.name, this.config.improvePluralization),
+        syncPluralName: this.pluralizeModelName(model)


should json syncPluralName be different than java visitor?
https://github.com/aws-amplify/amplify-codegen/pull/255/files#diff-8fdb1b5aed85eca745295520073edb2af47d27fdf8cff3173487f147b11e5fdaR761

good catch I think the java visitor needs fixed

lazpavel

LGTM

sebsto · 2021-11-28T20:23:16Z

hello, any chance to get this in the next CLI release ?

AaronZyLee

lgtm 🐰

lawmicha · 2022-01-31T16:37:05Z

any updates on this?

harsh62 · 2022-03-22T14:45:18Z

@marcvberg following up to see when can we expect this to be closed?

alharris-at · 2022-06-29T00:07:37Z

Hi @marcvberg could you reopen this PR against the main branch?

Nevermind, I was able to update myself. Feel free to ignore.

alharris-at · 2022-07-01T18:52:57Z

@marcvberg let's create an asana task to get this released in the next sprint.

marcvberg · 2022-07-01T18:56:49Z

@alharris-at Added to my tasks

sebsto · 2022-07-11T06:08:48Z

Hello, Any status when this will be merged. It blocks customer projects on this issue aws-amplify/amplify-swift#1443 (opened since Sept. 2021). This PR is open since Oct 2021. Can you help to prioritize this ?

AaronZyLee

The failing tests are probably due to the dependency change. You can rebase the main branch (or merge main) with the latest dependency versions and re-build the project.

AaronZyLee

Could you add a unit test for the schema.js generated by JS modelgen since the output is changed? Otherwise LGTM

packages/appsync-modelgen-plugin/src/__tests__/visitors/appsync-swift-visitor.test.ts

package.json

AaronZyLee

LGTM. This will be held until the studio CMS using the new model introspection schema as there are new fields introduced in schema.js

sebsto · 2022-10-13T13:36:17Z

Any chance to get this merged before re:Invent ? I am developing a workshop with Amplify. Current workshop instructions asks customer to manually modify the generated code. It would be great is this can be fixed by then. Thanks

lgtm-com · 2022-10-14T17:18:30Z

This pull request introduces 13 alerts when merging 5539ee5 into 00ba754 - view on LGTM.com

new alerts:

13 for Syntax error

alharris-at · 2022-10-19T22:06:20Z

@sebsto what platform are you writing your workshop for? We're testing this right now to verify if we're in a good place to merge, but it looks like Android/Flutter still won't have support for improvedPluralization after this PR from my current testing.

sebsto · 2022-10-20T04:07:41Z

@alharris-at workshop is for iOS / Swift
I currently have instructions guiding customers to modify the generated code

(and while I am at it, there is a warning about field ID being deprecated. but I should report that on another issue)

lawmicha · 2022-10-27T14:55:17Z

@alharris-at workshop is for iOS / Swift I currently have instructions guiding customers to modify the generated code

(and while I am at it, there is a warning about field ID being deprecated. but I should report that on another issue)

the warning about field ID being deprecated is a swift codegen warning. This is related to the new identifier field that is being created for the ability to create customizations of the primary key (renaming / composite, etc). To enable this, you can provide some instructions to the developer to toggle respectprimarykeyattributesonconnectionfield to true in the cli.json file before provisioning the backend (amplify push) and codegenerating the swift models (amplify codegen models)

For reference, the CPK feature docs is located over here aws-amplify/docs#4452

afern247 · 2022-12-28T17:35:24Z

The last comment on this PR was on October. Is someone actively working on this? I have an open issue reported: aws-amplify/amplify-swift#2460

sebsto · 2023-02-22T07:09:53Z

Thank you @marcvberg for the push. What are the next steps to get this merged ?

alharris-at

Q on the interface, otherwise looks sane to me given we understand the e2e testing a bit more.

alharris-at · 2023-02-27T18:42:40Z

...psync-modelgen-plugin/src/__tests__/visitors/__snapshots__/appsync-java-visitor.test.ts.snap

-@ModelConfig(pluralName = \\"Posts\\", type = Model.Type.USER, version = 1)
+@ModelConfig(listPluralName = \\"Posts\\", syncPluralName = \\"Posts\\", type = Model.Type.USER, version = 1)


Is this a breaking change to no longer also specify the 'pluralName'?

alharris-at · 2023-02-27T18:43:08Z

...psync-modelgen-plugin/src/__tests__/visitors/__snapshots__/appsync-java-visitor.test.ts.snap

@@ -1845,7 +1845,7 @@ import static com.amplifyframework.core.model.query.predicate.QueryField.field;

 /** This is an auto generated class representing the Post type in your schema. */
 @SuppressWarnings(\\"all\\")
-@ModelConfig(pluralName = \\"Posts\\")


Why do some of these not have the type/version as well?

sebsto · 2023-03-14T14:05:00Z

@marcvberg hello Marc, any steps to move forward and review @alharris-at comments above ? Thank you

sebsto · 2023-10-02T19:24:29Z

Looks like this issue has been fixed by aws-amplify/amplify-swift#3135

marcvberg requested a review from a team as a code owner October 7, 2021 21:41

marcvberg requested a review from a team October 7, 2021 21:41

dpilch requested changes Oct 8, 2021

View reviewed changes

...psync-modelgen-plugin/src/__tests__/visitors/__snapshots__/appsync-java-visitor.test.ts.snap Outdated Show resolved Hide resolved

dpilch previously approved these changes Oct 8, 2021

View reviewed changes

dpilch requested a review from AaronZyLee October 8, 2021 19:39

dpilch added the pending-release label Oct 8, 2021

AaronZyLee reviewed Oct 8, 2021

View reviewed changes

manueliglesias previously approved these changes Oct 8, 2021

View reviewed changes

AaronZyLee suggested changes Oct 21, 2021

View reviewed changes

AaronZyLee removed the pending-release label Oct 21, 2021

lazpavel reviewed Nov 18, 2021

View reviewed changes

lazpavel previously approved these changes Nov 18, 2021

View reviewed changes

AaronZyLee previously approved these changes Nov 30, 2021

View reviewed changes

lawmicha mentioned this pull request Jan 31, 2022

Amplify API query issue for table named Activity aws-amplify/amplify-swift#1505

Closed

yeung-wah force-pushed the master branch from fd7227c to 48b10f1 Compare February 24, 2022 18:22

lawmicha mentioned this pull request Mar 25, 2022

.paginatedList() query fails with Model that has name ending in y aws-amplify/amplify-swift#1707

Closed

alharris-at changed the base branch from master to main June 29, 2022 00:10

alharris-at requested a review from a team as a code owner June 29, 2022 00:10

AaronZyLee reviewed Jul 12, 2022

View reviewed changes

marcvberg force-pushed the improved-pluralization branch from 6100231 to 5bd5b09 Compare August 5, 2022 19:48

AaronZyLee suggested changes Aug 8, 2022

View reviewed changes

packages/appsync-modelgen-plugin/src/__tests__/visitors/appsync-swift-visitor.test.ts Outdated Show resolved Hide resolved

AaronZyLee suggested changes Aug 8, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

AaronZyLee previously approved these changes Aug 18, 2022

View reviewed changes

chrisbonifacio mentioned this pull request Sep 14, 2022

AppSyncGraphQLRequestFactory.buildQuery build wrong query aws-amplify/amplify-android#1814

Open

1 task

marcvberg dismissed AaronZyLee’s stale review via 95bf566 September 28, 2022 20:23

sebsto mentioned this pull request Oct 21, 2022

Xcode warnings - Upgrade pluralName aws-amplify/amplify-swift#2460

Closed

feature: support improved pluralization

65e5bfa

marcvberg force-pushed the improved-pluralization branch from db4ff43 to 65e5bfa Compare February 21, 2023 21:18

AaronZyLee approved these changes Feb 24, 2023

View reviewed changes

alharris-at reviewed Feb 27, 2023

View reviewed changes

This was referenced Aug 10, 2023

Pending Release: Use Improved Pluralization aws-amplify/amplify-swift#3135

Closed

feat: swift use improved pluralization #667

Merged

dpilch closed this Dec 18, 2023

		@ModelConfig(pluralName = \\"Posts\\", type = Model.Type.USER, version = 1)
		@ModelConfig(listPluralName = \\"Posts\\", syncPluralName = \\"Posts\\", type = Model.Type.USER, version = 1)

Feature: Use improved pluralization #255

Feature: Use improved pluralization #255

Conversation

marcvberg commented Oct 7, 2021 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

codecov-commenter commented Oct 8, 2021 • edited Loading

Codecov Report

AaronZyLee left a comment • edited Loading

Choose a reason for hiding this comment

manueliglesias left a comment

Choose a reason for hiding this comment

dcipoletti commented Oct 8, 2021

lawmicha commented Oct 14, 2021 • edited Loading

lawmicha commented Oct 18, 2021

HuiSF commented Oct 21, 2021

AaronZyLee commented Oct 21, 2021

AaronZyLee Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

HuiSF Oct 21, 2021

Choose a reason for hiding this comment

HuiSF Oct 21, 2021

Choose a reason for hiding this comment

marcvberg Nov 4, 2021

Choose a reason for hiding this comment

lawmicha commented Oct 22, 2021

lawmicha commented Nov 3, 2021

lazpavel Nov 18, 2021

Choose a reason for hiding this comment

marcvberg Nov 18, 2021

Choose a reason for hiding this comment

lazpavel left a comment

Choose a reason for hiding this comment

sebsto commented Nov 28, 2021

AaronZyLee left a comment

Choose a reason for hiding this comment

lawmicha commented Jan 31, 2022

harsh62 commented Mar 22, 2022

alharris-at commented Jun 29, 2022 • edited Loading

alharris-at commented Jul 1, 2022

marcvberg commented Jul 1, 2022

sebsto commented Jul 11, 2022

AaronZyLee left a comment

Choose a reason for hiding this comment

AaronZyLee left a comment

Choose a reason for hiding this comment

AaronZyLee left a comment

Choose a reason for hiding this comment

sebsto commented Oct 13, 2022

lgtm-com bot commented Oct 14, 2022

alharris-at commented Oct 19, 2022

sebsto commented Oct 20, 2022

lawmicha commented Oct 27, 2022 • edited Loading

afern247 commented Dec 28, 2022

sebsto commented Feb 22, 2023

alharris-at left a comment

Choose a reason for hiding this comment

alharris-at Feb 27, 2023

Choose a reason for hiding this comment

alharris-at Feb 27, 2023

Choose a reason for hiding this comment

sebsto commented Mar 14, 2023

sebsto commented Oct 2, 2023

marcvberg commented Oct 7, 2021 •

edited

Loading

codecov-commenter commented Oct 8, 2021 •

edited

Loading

AaronZyLee left a comment •

edited

Loading

lawmicha commented Oct 14, 2021 •

edited

Loading

AaronZyLee Oct 21, 2021 •

edited

Loading

alharris-at commented Jun 29, 2022 •

edited

Loading

lawmicha commented Oct 27, 2022 •

edited

Loading