aws-amplify / amplify-swift

A declarative library for application development using cloud services.
Apache License 2.0
454 stars 196 forks source link

[DataStore] Invalid AWSPhone values cause silent failure when saving model to DynamoDB #1393

Open HuiSF opened 3 years ago

HuiSF commented 3 years ago

Describe the bug

While using schemas with AWSPhone typed fields, if invalid phone numbers are provided, save API call succeeded, model saved into local DB, modal is not saved into DynamoDB due to DynamoDB value validation raised GraphQL error.

Original issue: https://github.com/aws-amplify/amplify-flutter/issues/822#issuecomment-904919041 amplify-android issue: https://github.com/aws-amplify/amplify-android/issues/1462

Schema example

type Person @model {
  id: ID!
  name: String!
  phone: AWSPhone
}

Steps To Reproduce

Steps to reproduce the behavior:
1. create a minimal app using amplify-android with above listed schema
2. Save model with invalid phone number
3. Observe

Expected behavior

The save API call should return error with clear error message from the GraphQL error. Invalid data should not persisted in local DB.

Amplify Framework Version

1.13.3

Amplify Categories

DataStore

Dependency manager

Swift PM

Swift version

5

CLI version

5.3.0

Xcode version

12.5.1

Relevant log output

No response

Is this a regression? (i.e. was this working before a version upgrade)

No response

Device

iPhone 12 simulators

iOS Version

iOS 14.5

Specific to simulators

No response

Additional context

GraphQL request

{
    "variables": {
        "input": {
            "phone": "1110004444",
            "name": "Test with Phone",
            "id": "6dce72c5-9eeb-4f0e-85aa-bc8350317e2e"
        }
    },
    "query": "mutation CreatePerson($input: CreatePersonInput!) {\n  createPerson(input: $input) {\n    id\n    name\n    phone\n    department {\n      id\n      name\n      org {\n        id\n        name\n        __typename\n        _version\n        _deleted\n        _lastChangedAt\n      }\n      __typename\n      _version\n      _deleted\n      _lastChangedAt\n    }\n    __typename\n    _version\n    _deleted\n    _lastChangedAt\n  }\n}"
}

GraphQL response

{
    "data": null,
    "errors": [{
        "path": null,
        "locations": [{
            "line": 1,
            "column": 23,
            "sourceName": null
        }],
        "message": "Variable 'phone' has an invalid value. Unable to parse `1110004444` as a valid phone number."
    }]
}
lawmicha commented 3 years ago

Hi @HuiSF, I codegenerated the schema and it doesn't contain any information that it is an AWSPhone so there may be a bit more work to support client side validation. We would first need to pass the information that it's a AWSPhone or similar to allow the library to know this.

extension Person {
 // ...
  public static let schema = defineSchema { model in
    let person = Person.keys

    model.pluralName = "People"

    model.fields(
      .id(),
      .field(person.name, is: .required, ofType: .string),
      .field(person.phone, is: .optional, ofType: .string), // library does not know that this is an AWSPhone
      .field(person.createdAt, is: .optional, isReadOnly: true, ofType: .dateTime),
      .field(person.updatedAt, is: .optional, isReadOnly: true, ofType: .dateTime)
    )
    }
}
lawmicha commented 3 years ago

currently the possible types are

public enum ModelFieldType {

    case string
    case int
    case double
    case date
    case dateTime
    case time
    case timestamp
    case bool
    case `enum`(type: EnumPersistable.Type)
    case embedded(type: Codable.Type, schema: ModelSchema?)
    case embeddedCollection(of: Codable.Type, schema: ModelSchema?)
    case model(name: ModelName)
    case collection(of: ModelName)
HuiSF commented 3 years ago

Hi @lawmicha thanks for looking!

AWSPhone is usable while defining schema. all scalar types are listed here. Model gen converts some of the scalar types defined in GraphQL schema to string, e.g. AWSPhone and AWSEmail.

When syncing data to cloud, AppSync validates values according to the GraphQL schema which raised the error.

lawmicha commented 3 years ago

We'll have to decide if we should be doing validation on the client side for these fields. I think this is right approach since it should improve the developer code to code against the result of the save operation, and leave the error handler logic scenario to the remaining edge cases. On JS, the schema definition is available to check that it is an AWS type (AWSPhone, AWSEmail, etc) and the existing logic already does some validation on the fields of these types.

The next step is to define the codegen spec changes required to store the information for native platforms. This may be in the form of an optional associated type on the string case:

.field(person.phone, is: .optional, ofType: .string(AWSPhone.self),),

And identify what is the client side validation we should be doing (perhaps a light version of the AWSPhone requirements / use NSDataDectector which is also what developers can use right now), and identify the DX for how to handle the failed validation case.