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 support for Enum and non-model types #334

Merged
merged 12 commits into from
Mar 5, 2020

Conversation

drochetti
Copy link
Contributor

@drochetti drochetti commented Feb 21, 2020

Issues: #111 #240 #246 #318 #314 (Partially, API is still missing)

Notes:

  • added support for SQLite to handle Enums
  • added support for SQLite to handle non-model types:
    • custom Codable structs
    • codable arrays
  • added tests to make sure the right values are represented as SQLite bindings

Tests:

  • added a model that has all types of fields to make it easier to
    visualize how data is represented on SQLite

Pending:

  • More documentation
  • GraphQL cleanup on API category
  • More tests
  • Merge with the latest Sync Engine changes

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

**Notes:**

- added support for SQLite to handle Enums
- added support for SQLite to handle non-model types:
  - custom Codable structs
  - codable arrays
- added tests to make sure the right values are represented as SQLite
bindings

**Tests:**

- added a model that has all types of fields to make it easier to
visualize how data is represented on SQLite

**Pending:**

- More documentation
- GraphQL clenup on API category
- More tests
- Merge with the latest Sync Engine changes
@drochetti drochetti self-assigned this Feb 21, 2020
@drochetti drochetti added the datastore Issues related to the DataStore category label Feb 21, 2020
@lawmicha
Copy link
Contributor

Non-blocking comment, but just want to share my learnings for the backend provisioning. When i provisioned the backend with NonModel type in the schema, it does not create any mutations or queries, and just defines the type. Seems like the usecase is for attaching it to an existing type that contains the model directive. for example:

type Todo @model {
  id: ID!
  name: String!
  description: String
  innerTodo: JustTodo
}

type JustTodo {
  name: String!
  description: Int
}
mutation createTodo($input: CreateTodoInput!) {
  createTodo(input: $input) {
    description
  	innerTodo {
      name
      description
    }
  }
}

Copy link
Contributor

@lawmicha lawmicha left a comment

Choose a reason for hiding this comment

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

awesome to see the support for enum and non-model types within a model. Looks good to me, I do want to follow up with a sample which have every scalar types in a Model and an non-model type as a field in the object type in a schema, and run through amplify codegen models. I had some thoughts about this for being in an integration test on top of this PR but the blocker might be that amplify CLI is not up to date yet.

/// case published
/// }
/// ```
public protocol ModelEnum: Codable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To @palpatim (and anyone else interested).

We talked about relying solely on the RawRepresentable for this. However, I could not make it work properly with our current StorageEngine implementation. RawRepresentable has an associatedtype (named RawValue) which blocks me from doing RawRepresentable.Type for ModelFieldDefinition in the schema (.enum(MyEnum.Type)).

I'm open to suggestions!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@palpatim as we talked offline, I tried making enums just conform to Codable and not rely on any specific protocol. I faced two issues:

  1. Given the Codable freedom, users could have their enums being represented by Int instead of String, which then breaks our logic to save it to a TEXT column in SQLite. It would be possible but our code that handle Any type would become more complex.
  2. Related to the previous point, there's one place in our code (which I intend to simplify, but it will require work) that does a Any.Type -> ModelFieldType conversion, then I had no way to tell .type (or .customType) from .enum unless I drop .enum entirely.

Those two would be solved by an isEnum(Any.Type) or is ValueEnumerable.Type (like a proposal on Swift Evolution). But for now I believe we will need a custom protocol in order to keep our serialization code sane and move forward.

So my suggestion is we go with a type like the ModelEnum I defined in the first version of this PR, that defines a more strict contract for enums (i.e. it has to be represented by a String) and find a better name for it like you suggested. Options that I discussed with you and some other members of the team:

  1. RawStringRepresentable
  2. StringRepresentable
  3. PersistableEnum

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the thoughtful due diligence on this. After discussing and my own belated realization that this is primarily a codegen/model-authoring structure, I'd favor a name like ModelValueStringRepresentable or something that makes it (painfully) obvious that we really only want to use this in a narrow set of circumstances. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm imho ModelEnum... doesn't sound that bad, considering we have models that conform to Model, so I'm not quite sure why this was veto'ed.

But if we are looking at this holistically, and considering it's a protocol (and protocols should be -able), and riffing off of one of drochetti's suggestions, how about EnumPersistable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing and the feedback John. Let's do this:

  • I really believe (and hope) that Swift will add some type of marker protocol do all enums in the future given the community engagement in such feature (ValueIterable is the current proposal as you can see here: Deriving collections of enum cases swiftlang/swift-evolution#114 (comment)). So this is really something that I hope we can remove in the future
  • The protocol we're introducing can (should) only be used in enums, so I like the idea of naming it in a way we communicate that constraint, so EnumPersistable sounds good to me
  • Our goal is to remove this protocol in the future in favor of either: Codable or ValueIterable, so I think we all agree on a future refactor of this
  • Last but not least, this is controlled by the codegen provided by the CLI since handwritten models is not something we support yet (it's possible but there's no documentation around it), so if we decide to rename it in the future the impact is not too big. We can also deprecate it so customer don't break, etc

@drochetti drochetti requested review from palpatim and wooj2 February 25, 2020 18:53
- add tests for `AnyEncodable`
- renamed variables for improved readability
- improved error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants