fix(amplify-codegen-appsync-model-plugin): pass targetName for hasOne relationships

[lawmicha]

Issue #, if available:
aws-amplify/amplify-swift#920

Description of changes:
This adds the targetName for hasOne relationships, closely mirroring the same logic applied for belongsTo.

Testing done
schema

type Project2 @model {
  id: ID!
  name: String
  teamID: ID! // Explicit field for Team association
  team: Team2 @connection(fields: ["teamID"])
}

type Team2 @model {
  id: ID!
  name: String!
}

Generated swift schema class without this change:

// swiftlint:disable all
import Amplify
import Foundation

extension Project2 {
  // MARK: - CodingKeys 
   public enum CodingKeys: String, ModelKey {
    case id
    case name
    case teamID
    case team
  }
  
  public static let keys = CodingKeys.self
  //  MARK: - ModelSchema 
  
  public static let schema = defineSchema { model in
    let project2 = Project2.keys
    
    model.pluralName = "Project2s"
    
    model.fields(
      .id(),
      .field(project2.name, is: .optional, ofType: .string),
      .field(project2.teamID, is: .required, ofType: .string),
      .hasOne(project2.team, is: .optional, ofType: Team2.self, associatedWith: Team2.keys.id)
    )
    }
}

Generated schema with this change:

.hasOne(project2.team, is: .optional, ofType: Team2.self, associatedWith: Team2.keys.id, targetName: "teamID")

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

[codecov]

Codecov Report

Merging #6031 (c0122e5) into master (db9bf58) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #6031    +/-   ##
========================================
  Coverage   58.21%   58.21%            
========================================
  Files         440      440            
  Lines       20160    20161     +1     
  Branches     3988     3783   -205     
========================================
+ Hits        11736    11737     +1     
- Misses       7609     7629    +20     
+ Partials      815      795    -20     

Impacted Files	Coverage Δ
...model-plugin/src/visitors/appsync-swift-visitor.ts	97.51% <ø> (ø)
...sync-model-plugin/src/utils/process-connections.ts	91.02% <100.00%> (+0.11%)	⬆️
packages/amplify-util-mock/src/api/api.ts	0.00% <0.00%> (ø)
packages/graphql-mapping-template/src/print.ts	35.29% <0.00%> (ø)
packages/amplify-util-mock/src/storage/storage.ts	0.00% <0.00%> (ø)
...ges/amplify-util-mock/src/CFNParser/stack/index.ts	0.00% <0.00%> (ø)
...es/amplify-util-mock/src/api/resolver-overrides.ts	0.00% <0.00%> (ø)
...es/graphql-transformer-core/src/stripDirectives.ts	35.29% <0.00%> (ø)
.../amplify-cli-core/src/state-manager/pathManager.ts	67.85% <0.00%> (ø)
.../amplify-util-mock/src/utils/lambda/loadMinimal.ts	0.00% <0.00%> (ø)
... and 11 more

---

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 296ae55...c0122e5. Read the comment docs.

[ammarkarachi]

@AaronZyLee Can you review?

[AaronZyLee]

LGTM but I am still investigating and looking out for a general solution for other SDKs. And what I am concerned for this solution is that teamID is not optional when you create a Project, not only in modelgen but also in CreateProjectInput in appsync. When refering to A has one B, it means that A does not necessarily need B but B is dependent on A. If we change the teamID to an optional field, it will throw the following error by transfomer:

InvalidDirectiveError: All fields provided to an @connection must be non-null scalar or enum fields.

The reason is that when referring to an identifier in @connection(fields:[]) the param should not be nullable.

On the other hand, I am not sure our document has a good guide on how to design a one-to-one schema. Why do we need an identifier in a HAS_ONE relation? Do we have any scenario that using project.teamID is better than using project.team.id?

In the HAS_MANY and BELONGS_TO bi-way connection(one to many), however, it is really necessary to use an identifier. Say post has many comments, and in comment schema we add a postID as well as a @key to define postID as a GSI, from which the post can have a query to get all comments for post.comments.

In conclusion, I think a better approach is that we do not use identifier, aka teamID from the example schema, in a one to one relation. Overall, the following schema should be an expected one-to-one schema in our guide, which causes less trouble. And one thing to mention is that the following pattern is also used in the Amplify Admin UI.

type Project @model {
  id: ID!
  name: String
  team: Team @connection
}

type Team @model {
  id: ID!
  name: String!
}

[lawmicha]

@AaronZyLee Thanks for reviewing, will loop back here to continue the conversation regarding your comment. Can we merge this in first? I believe your approval doesn't have write access yet. @attilah / @ammarkarachi or someone with write access can you also approve to merge this in?

[lawmicha]

LGTM but I am still investigating and looking out for a general solution for other SDKs. And what I am concerned for this solution is that teamID is not optional when you create a Project, not only in modelgen but also in CreateProjectInput in appsync. When refering to A has one B, it means that A does not necessarily need B but B is dependent on A. If we change the teamID to an optional field, it will throw the following error by transfomer:

InvalidDirectiveError: All fields provided to an @connection must be non-null scalar or enum fields.

The reason is that when referring to an identifier in @connection(fields:[]) the param should not be nullable.

I also noticed this behavior and believe a non optional team ID is correct. A team can exist without a project, but a project cannot exist without a team. you should never have orphaned projects, and always associated with a team. If you want to have a orphaned project, you may as the developer create a dummy placeholder team to associate your orphaned projects.

On the other hand, I am not sure our document has a good guide on how to design a one-to-one schema. Why do we need an identifier in a HAS_ONE relation? Do we have any scenario that using project.teamID is better than using project.team.id?

In the HAS_MANY and BELONGS_TO bi-way connection(one to many), however, it is really necessary to use an identifier. Say post has many comments, and in comment schema we add a postID as well as a @key to define postID as a GSI, from which the post can have a query to get all comments for post.comments.

In conclusion, I think a better approach is that we do not use identifier, aka teamID from the example schema, in a one to one relation. Overall, the following schema should be an expected one-to-one schema in our guide, which causes less trouble. And one thing to mention is that the following pattern is also used in the Amplify Admin UI.

type Project @model {
  id: ID!
  name: String
  team: Team @connection
}

type Team @model {
  id: ID!
  name: String!
}

I believe we need to support both explicit fields using the key directive and fields parameter in connections and implicit fields using the older connection directive (without the name parameter/omitting all parameters like u have above). In your above example. But how I understood the documentation was that explicit field with key directive and fields is the newer version of the connection directive usage and is the recommended way

[ammarkarachi]

LGTM!

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.