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

Add Embeddable type to store schema info for custom types #539

Merged
merged 2 commits into from
Jun 16, 2020

Conversation

lawmicha
Copy link
Contributor

@lawmicha lawmicha commented Jun 10, 2020

Issue #, if available:

Description of changes:

  • This adds Embbeded protocol to return the schema information for embedded types. This means the code generated schema for an embedded type should look something like this:
class Category: Embedded {
...
}
extension Category {
    public enum CodingKeys: CodingKey {
        case name
        case color
    }
    public static let keys = CodingKeys.self

    // store info about the field's name, nullability, and their types.
    public static let schema = defineSchema { embedded in
        let category = Category.keys
        embedded.fields(.field(category.name, is: .required, ofType: .string),
                       .field(category.color, is: .required, ofType: .embedded(type: Color.self)))
    }
}
  • Also introduced a .embeddedCollection case to hold embbeded types which are arrays. This was required when checking type at runtime.
    public var embeddedType: Embedded.Type? {
        switch type {
        case .embedded(let type), .embeddedCollectiion(let type):
           // the `type` here is always castable to the Embedded.Type. 
            if let embeddedType = type as? Embedded.Type {
                return embeddedType
            }

Testing done so far

  • selection set verification
  • graphql query variables input, graphQLInput
  • sample app without conflict resolution enabled and making calls to API.create
  • sample app with conflict resolution enabled and DataStore.save is successfully and is synced to the cloud

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

@lawmicha lawmicha requested a review from drochetti June 10, 2020 23:07
Copy link
Contributor

@drochetti drochetti left a comment

Choose a reason for hiding this comment

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

Overall I think the approach is mostly correct but we should not duplicate all the *Schema, *Field types we have for models.

We should either:

  1. Extract that to a protocol and make it more generic
  2. or use the ModelSchema, ModelField for the custom types as well and add a modifier saying it's not a model (or it's an embedded type)

// MARK: - CustomCodable

/// All persistent custom types should conform to the CustomCodable protocol.
public protocol CustomCodable: Codable {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need a better name for CustomCodable. ORMs typically call these "Embedded" types, maybe we could follow that?

@jpignata also mentioned the non-model thing we keep talking about is not clear, and I agree.

@lawmicha lawmicha force-pushed the fix/non-model-types branch 2 times, most recently from 577b0fe to 376774f Compare June 12, 2020 01:29
@lawmicha lawmicha force-pushed the fix/non-model-types branch from 376774f to d93b957 Compare June 12, 2020 01:35
@lawmicha lawmicha marked this pull request as ready for review June 12, 2020 02:32
@lawmicha lawmicha requested a review from drochetti June 12, 2020 02:32
@lawmicha
Copy link
Contributor Author

lawmicha commented Jun 12, 2020

saw an issue (that doesn't seem critical to this PR) while testing, opening issue #541

@drochetti drochetti added this to the 1.0.2 milestone Jun 12, 2020
Copy link
Contributor

@drochetti drochetti left a comment

Choose a reason for hiding this comment

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

Looks good! I left a couple of minor comments, but overall it looks great :)


// MARK: - CustomCodable

/// All persistent custom types should conform to the CustomCodable protocol.
Copy link
Contributor

Choose a reason for hiding this comment

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

update description.

also, maybe it would be good to expand on this a bit and explain the different between Embedded and Model

Comment on lines 199 to 204
if case .embedded = type {
return true
}
if case .embeddedCollection = type {
return true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you could potentially combine both using a , in the same if clause, or also use a switch where the default returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think if case .x = type, case .y = type is checking for both case and will never be true. not completely sure if that is the case when i saw a test failed, so i switched to using the switch statement instead

// MARK: - CustomCodable

/// All persistent custom types should conform to the CustomCodable protocol.
public protocol Embedded: Codable {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: maybe worth considering calling the protocol Embeddable.

It suits well what the industry usually uses (see Hibernate approach) and also the iOS naming patterns.

An Embeddable is .embedded(type:) in a Model. I can see that making way more sense than "custom type" or "non-model type". =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that sounds good, i will make this change!

@lawmicha lawmicha changed the title Add CustomCodable to store schema info for custom types Add Embedded type to store schema info for custom types Jun 15, 2020
@lawmicha
Copy link
Contributor Author

related Codegen aws-amplify/amplify-cli#4545

@lawmicha lawmicha self-assigned this Jun 15, 2020
@lawmicha lawmicha requested a review from drochetti June 15, 2020 21:09
@lawmicha lawmicha added api Issues related to the API category datastore Issues related to the DataStore category labels Jun 15, 2020
@lawmicha lawmicha merged commit 7b5611b into master Jun 16, 2020
@lawmicha lawmicha deleted the fix/non-model-types branch June 16, 2020 14:58
@lawmicha lawmicha changed the title Add Embedded type to store schema info for custom types Add Embeddable type to store schema info for custom types Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues related to the API category datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants