-
Notifications
You must be signed in to change notification settings - Fork 247
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
feat(datastore): implement ModelIndex #1366
feat(datastore): implement ModelIndex #1366
Conversation
@@ -152,6 +155,10 @@ class Blog extends Model { | |||
modelSchemaDefinition.name = "Blog"; | |||
modelSchemaDefinition.pluralName = "Blogs"; | |||
|
|||
modelSchemaDefinition.indexes = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a difference between an empty array and an array with the id
field marked with @primaryKey
? Wondering if we need to always include the primary key field or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing :)
This is simply following GraphQL transformer + codegen convention of how to describe an index, e.g. @primaryKey
@model
with normal id
as the default primary key
{ name: 'key', arguments: { fields: [ 'id' ] } }
@model
with modelID
annotated by @primaryKey
{ name: 'key', arguments: { fields: [ 'modelID' ] } }
@model
with productId
annotated by @primaryKey
+ sortKeyFields
{
name: 'key',
arguments: { fields: [ 'productId', 'productName', 'warehouseId', 'region' ] }
}
And @index
@model
has postId
annotated with @index
+ sortKeyFields
{
name: 'byPost',
arguments: { fields: [ 'postId', 'content' ] }
}
@@ -69,34 +61,16 @@ class Person extends Model { | |||
} | |||
} | |||
|
|||
TemporalDateTime? get createdAt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to createdAt
/updatedAt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a custom type, should not contain model meta fields. I think the original generated code was outdated...
} | ||
} | ||
/* | ||
* Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is registering as a change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this PR converts this file from CRLF to LF mode (The first commit of this PR).
If you view the second commit it shows the actual changes on this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one question about a diffing issue, but otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few nit comments but lgtm
String toString() => 'ModelIndex(name: $name, fields: $fields)'; | ||
|
||
@override | ||
bool operator ==(Object other) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we start using the AWSEquatable
mixin instead of implementing these everywhere? It is currently in amplify_flutter, so you would have to move it if you want to use it. I don't think you have to use it for this PR, but I want to discuss if we want to start using it moving forward.
@dnys1 I think you added AWSEquatable. Any reason we can't move that to amplify_core and start using it throughout all packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, wasn't aware of this mixin. Can we also use it with generated models? @dnys1
Codecov Report
@@ Coverage Diff @@
## feat/custom-pk #1366 +/- ##
==================================================
- Coverage 47.64% 47.51% -0.13%
==================================================
Files 245 246 +1
Lines 10224 10251 +27
==================================================
Hits 4871 4871
- Misses 5353 5380 +27
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* Convert CRLF to LF * feat(datastore): implement ModelIndex * chore(datastore): update tests to cover model indexes * Improve doc
* Convert CRLF to LF * feat(datastore): implement ModelIndex * chore(datastore): update tests to cover model indexes * Improve doc
* Convert CRLF to LF * feat(datastore): implement ModelIndex * chore(datastore): update tests to cover model indexes * Improve doc
* Convert CRLF to LF * feat(datastore): implement ModelIndex * chore(datastore): update tests to cover model indexes * Improve doc
Issue #, if available:
#1426
Description of changes:
Currently, codegen generated Dart models don't contain model index information. This PR is adding relevant interface and implementation to support upcoming changes in codegen to provide index information in model schema.
Note: this PR doesn't include Flutter native implementation to convert serialize model indexes into native objects. This implementation will be added in a separate PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.