feat(api): adds selection set customization #2526
Conversation
drochetti
changed the title
* Update PULL_REQUEST_TEMPLATE.md * Update PULL_REQUEST_TEMPLATE.md * Update PULL_REQUEST_TEMPLATE.md
…plify/amplify-swift into feature/api-selection-set
| let schema = ModelRegistry.modelSchema(from: associatedModelName) { | ||
| let child = SelectionSet(value: .init(name: field.name, fieldType: .model)) | ||
| child.withModelFields(schema.graphQLFields) | ||
| child.withModelFields(schema.primaryKey.fields) |
There was a problem hiding this comment.
this will break existing setups that use the previously generated models that do not have the LazyReference encapsulation since the associated model needs to have the full selection set for decoding. To make it backwards compatible, i passed in a flag called "primaryKeysOnly" to this method to control this to be false for the current codegen.
lawmicha@2585590#diff-75edd460680ee3bcc5a0b05a8f77b5a5be907e55532914649870c0d29948ce55
if primaryKeysOnly {
child.withModelFields(schema.primaryKey.fields, primaryKeysOnly: primaryKeysOnly)
} else {
child.withModelFields(schema.graphQLFields, primaryKeysOnly: primaryKeysOnly)
}There was a problem hiding this comment.
Sounds good! I reviewed your approach and will keep this PR like this since it'll be fixed once you apply yours on top of it.
| var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelSchema: modelType.schema, operationType: .query) | ||
| documentBuilder.add(decorator: DirectiveNameDecorator(type: .list)) | ||
|
|
||
| if let modelPath = modelType.rootPath as? ModelPath<M> { | ||
| let associations = includes(modelPath) | ||
| documentBuilder.add(decorator: IncludeAssociationDecorator(associations)) | ||
| } |
There was a problem hiding this comment.
For the implementation of these APIs, i checked if the modelType.rootPath can be unwrapped at the start of this method, because if it can be unwrapped means that it is using the codegen with lazy reference. If it is using codegen with lazy reference, then pass in primaryKeysOnly: true to ModelBasedGraphQLDocumentBuilder (#2583), otherwise keep primaryKeysOnly false for previous codegen version.
let primaryKeysOnly = (modelType.rootPath != nil) ? true: falseThis is to account for when includes is the default value && new codegen is used then primaryKeysOnly == true. when old codegen is used, it should be primaryKeysOnly == false
There was a problem hiding this comment.
That makes sense. So should I leave it like this here since your PR will change it?
AmplifyPlugins/Core/AWSPluginsCore/Model/GraphQLRequest/GraphQLRequest+Model.swift
Show resolved
Hide resolved
lawmicha
left a comment
lawmicha
left a comment
There was a problem hiding this comment.
Of course, feel free to use an alternative naming/approach to primaryKeysOnly flag in the suggestions above ^
| case unknown(ErrorDescription, RecoverySuggestion, Error) | ||
| } | ||
|
|
||
| extension DataError: AmplifyError { |
There was a problem hiding this comment.
we currently have CoreError which is returned on List operations. I think we should merge the two into one, and name it either CoreError or DataError as you have it here
| public func id(_ name: String = "id") -> FieldPath<String> { | ||
| FieldPath(name: name, parent: self) | ||
| } |
There was a problem hiding this comment.
Is there a special use case for this or just visually appealing to have it codegenerated as "id"? When hand-writing the schema with composite keys, both keys were not "id" so they are both string(<fieldName>), which means in this case that the primary key of a model with composite key doesn't show which of the fields are its primary keys in the ModelPath extension. Should we just remove this completely? Currently, I haven't added the check for "is primaryKey and field value is 'id'" and simply generated "string()" for all string fields including properties called "id"
There was a problem hiding this comment.
That's a good callout. It made sense before when we had ids based on the naming convention. That not being the case anymore calls for a different approach, I think we can simply use the string() function.
| FieldPath(name: name, parent: self) | ||
| } | ||
|
|
||
| public func string(_ name: String) -> FieldPath<String> { |
There was a problem hiding this comment.
missing FieldPath<Double>
There was a problem hiding this comment.
Thanks for catching that!
|
The changes from this PR has been merged with #2583 to |
4 participants
Custom selection set for GraphQL queries in the API category
This feature will enable customers to define which associations will be included in selection set when querying data using the static typed models. Previously developers had no control over that and had to fallback to untyped/string-based GraphQL queries when they needed more control. This feature enables fine control with full type information of which associations should be included in a query.
A quick example of the added capability is represented in the following code snippet:
Developers can go as deep as needed and/or add multiple associations. Next I'll go into more details on how this was accomplished to provide folks reviewing this PR more information about the implementation details and decisions.
Check points: (check or cross out if not relevant)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.