fix(amplify): DataStore query fix column missing issue for @connection belongsTo schema #885
Conversation
Codecov Report
@@ Coverage Diff @@
## main #885 +/- ##
==========================================
+ Coverage 64.89% 64.94% +0.04%
==========================================
Files 862 863 +1
Lines 34182 34227 +45
==========================================
+ Hits 22184 22228 +44
- Misses 11998 11999 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| extension ModelKey { | ||
| public var modelName: String { "" } |
There was a problem hiding this comment.
document the behavior. Why is it empty by default? (i.e. explain the backward compatibility)
...nsCoreTests/Model/Decorator/AuthRuleDecorator/ModelWithOwnerAuthAndGroupWithGroupClaim.swift
Outdated
Show resolved
Hide resolved
| /// - fields are wrapped with `items` | ||
| /// - check if generated GraphQL variables is valid: | ||
| /// - it contains the correct columnName `commentPostId` | ||
| func testListGraphQLQueryFromSimpleModelGivenQueryPredicateOnConnectedField() { |
There was a problem hiding this comment.
A new test is added here to test GraphQLList on connected field
drochetti
left a comment
drochetti
left a comment
There was a problem hiding this comment.
thanks for fixing this! remember not to merge into main until we have more clarify on the CL release with the codegen changes.
AmplifyPlugins/Core/AWSPluginsCoreTests/Model/GraphQLDocument/GraphQLListQueryTests.swift
Outdated
Show resolved
Hide resolved
618coffee
changed the title
618coffee
changed the title
| class CodingKeysTests: XCTestCase { | ||
|
|
||
| func testSchemaHasCorrectColumnName() throws { | ||
| ModelRegistry.register(modelType: Comment.self) |
There was a problem hiding this comment.
I believe that these tests will have side effects on other tests running in the suite.
After we register these models, how do we reset ModelRegistry? -- seems like we need to add a tearDown() function with calls ModelRegistry.reset()?
There was a problem hiding this comment.
To fix, we should add something like:
override func tearDown() {
guard let api = Amplify.API as? Resettable else {
XCTFail("API Needs to reset between tests")
return
}
api.reset(onComplete: { })
XCTAssert(ModelRegistry.models.isEmpty)
}
It's not ideal that we have to use Amplify.API, but ModelRegistry is internal (and rightfully so). In any case, checking to make sure models.isEmpty before proceeding is a good way to double check that a side effect of calling Amplify.API is actually reseting the ModelRegistry.
…onnection hasMany schema (#885)" This reverts commit 028a707.
| let commentQPO: QueryPredicateOperation = Comment.keys.post == "5678" | ||
| XCTAssertEqual(commentQPO.field, "commentPostId") | ||
| } | ||
|
|
There was a problem hiding this comment.
To add test coverage we, should probably add a test for the MutationEvent
func testInternalModelType() throws {
//arrange not required
let mutationEvent = MutationEvent.keys.inProcess == true
XCTAssertEqual(mutationEvent.field, "inProcess")
}
…onnection hasMany schema (#885)" (#963) This reverts commit 028a707. Co-authored-by: Guo <48600426+DongQuanRui@users.noreply.github.com>
| } | ||
| switch modelSchema.field(withName: stringValue)?.association { | ||
| case .belongsTo(_, let targetName): | ||
| return targetName ?? stringValue |
There was a problem hiding this comment.
We seem to have a unit tests which tests when targetName is not nil, but we should add a test where targetName is nil, so that it just returns the regular stringValue.
618coffee
changed the title
|
I believe this is reverted, see #965 for more details |
5 participants
Description of changes:
Existing issue
Take this schema as example:
Failure occurs when trying to call
Post.keys.useris picking up thestringValueofcase user, so that the generated SQL statement isBut Post.keys.user has a
belongs toassociation withtargetName: "postUserId", the correct SQL statement should beChange
CodingKey should check if there is type
.belongsToassociation, if yes, return targetName, otherwise returnstringValueAnd also the CodeGen Model should contains the computed property that returns
modelNameso thatmodelSchemacan be retrieved fromModelRegistry. Here is the PR by Diego@: aws-amplify/amplify-cli#5857By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.