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(modelgen-swift): respect index sk fields in associate fields #542

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AaronZyLee
Copy link
Contributor

@AaronZyLee AaronZyLee commented Feb 3, 2023

Description of changes

  • The associatedWith fields now respect the index sortKeyFields in explicit uni hasMany relation. This change only affects swift modelgen.
  • Rewrite the flag value parsing for processDirectives. Add new type definition and default value for directive process configuration.

Issue #, if available

Fix #539

Description of how you validated changes

yarn test
amplify-dev codegen models

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • Breaking changes to existing customers are released behind a feature flag or major version update
  • Changes are tested using sample applications for all relevant platforms (iOS/android/flutter/Javascript) that use the feature added/modified

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

@AaronZyLee AaronZyLee requested a review from a team as a code owner February 3, 2023 22:52
@AaronZyLee AaronZyLee marked this pull request as draft February 3, 2023 22:52
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #542 (f0bc596) into main (181480d) will increase coverage by 0.02%.
The diff coverage is 96.55%.

📣 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     #542      +/-   ##
==========================================
+ Coverage   85.69%   85.71%   +0.02%     
==========================================
  Files         148      148              
  Lines        7380     7364      -16     
  Branches     1962     1958       -4     
==========================================
- Hits         6324     6312      -12     
+ Misses        959      955       -4     
  Partials       97       97              
Impacted Files Coverage Δ
...-plugin/src/visitors/appsync-typescript-visitor.ts 82.89% <0.00%> (+1.60%) ⬆️
...nc-modelgen-plugin/src/utils/process-belongs-to.ts 94.11% <100.00%> (+0.17%) ⬆️
...odelgen-plugin/src/utils/process-connections-v2.ts 92.20% <100.00%> (-0.20%) ⬇️
...sync-modelgen-plugin/src/utils/process-has-many.ts 90.80% <100.00%> (+0.44%) ⬆️
...psync-modelgen-plugin/src/utils/process-has-one.ts 96.15% <100.00%> (+3.84%) ⬆️
...delgen-plugin/src/visitors/appsync-dart-visitor.ts 98.11% <100.00%> (-0.01%) ⬇️
...delgen-plugin/src/visitors/appsync-java-visitor.ts 94.78% <100.00%> (-0.03%) ⬇️
...-plugin/src/visitors/appsync-javascript-visitor.ts 97.72% <100.00%> (-0.15%) ⬇️
...ugin/src/visitors/appsync-json-metadata-visitor.ts 95.74% <100.00%> (-0.14%) ⬇️
...rc/visitors/appsync-model-introspection-visitor.ts 98.66% <100.00%> (-0.06%) ⬇️
... and 2 more

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

@AaronZyLee AaronZyLee changed the title fix: respect index sk fields in associate fields fix(modelgen-swift): respect index sk fields in associate fields Feb 8, 2023
@AaronZyLee AaronZyLee marked this pull request as ready for review February 8, 2023 22:22
Copy link
Contributor

@phani-srikar phani-srikar left a comment

Choose a reason for hiding this comment

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

Minor comments

// Configuration used for processing directives
export type CodeGenDirectiveProcessConfig = {
// This flag is going to be used for using custom primary key feature
isCustomPKEnabled: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean that there is a @primaryKey present in the schema or just that if it's present, we will codegen differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is determined by the feature flag value rather than the presence of the directive. If the value is true and the schema is a CPK (the @primaryKey is attached to non-id field/id PK with SK defined), it will codegen differently

return [otherSideConnectedField];
const sortKeyFieldNames: string[] = otherSideConnectedDir?.arguments.sortKeyFields ?? [];
return shouldRespectSortKeyFieldsOfIndexInAssociatedWithFields
? [otherSideConnectedField, ...sortKeyFieldNames.map(sk => connectedModel.fields.find(f => f.name === sk)!)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ! necessary in the find?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the sortKeyField should exist in connected when its field name is provided. Otherwise it is not a valid schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swift Model Codegen for Unidirectional Has-Many AssociatedFields Missing Index fields
3 participants