aws-amplify / amplify-android

The fastest and easiest way to use AWS from your Android app.
https://docs.amplify.aws/lib/q/platform/android/
Apache License 2.0
245 stars 115 forks source link

[GraphQL] Optional ID fields using a @BelongsTo relationship crash application with NullPointerException #2431

Closed srimalaya closed 1 year ago

srimalaya commented 1 year ago

Before opening, please confirm:

Language and Async Model

Kotlin - Coroutines

Amplify Categories

GraphQL API

Gradle script dependencies

```groovy // Put output below this line implementation "com.amplifyframework:aws-api:2.8.1" implementation "com.amplifyframework:core-kotlin:2.8.1" // Also tried with V1 implementation "com.amplifyframework:aws-api:1.38.3" implementation "com.amplifyframework:core-kotlin:0.22.3" ```

Describe the bug

Please take a look at the GraphQL Schema attached below. See that it has ID fields which are optional (nullable). Also see that fields like customer_id, animal_id, and measurement_id are linked using @BelongsTo relation.

Here is the bug: When a mutation is being triggered, I noticed that the @BelongsTo relation is being enforced as not null in the AppSyncGraphQLRequestFactory.java file. This causes a crash when I set a nested model to null. For example, if you follow the reproduction steps provided below, you can see that while saving note, customer is required but animal and measurement are supposed to be optional fields. But, when I leave either of them out, it causes an app crash.

Our previous app was built using Amplify V1 and it used to work just fine on the same schema so I initially thought it was a bug with Amplify V2. However, I have tested this schema with both V1 and V2 and the result stays the same, i.e., the app crashed with NullPointerException. This functionality is important to us. For example, we want every model to be able to have many Notes. This makes it obvious that each note cannot have a value for each model that it belongs to.

I am 100% certain that this feature used to work before on a similar schema. And I am certain it works or used to work on both iOS and JavaScript versions of our app.

My current way of handling this issue is to use a SimpleGraphQL request and use a custom mutation document as I would on the AppSync console, which is a huge headache to keep track of as we keep scaling the app up to use more models.

Reproduction steps (if applicable)

  1. Set up amplify env using the schema provided or using a similar schema with a model containing optional ID fields which are related to another model using @BelongsTo relationship.
  2. Using GraphQL API, create a customer, an animal, and a note in a similar fashion to provided code.
  3. Note how the note fails to get created and throws the Android Runtime error that is provided in the log output.
  4. This happens because note also has another optional field named measurement_id which is linked to Note using the @BelongsTo relationship

Code Snippet

// Put your code below this line.

        val customer: Customer = Customer.builder()
            .id("b226c01d-b12c-4a0a-913c-51edfecc134b")
            .name("lorem ipsum dolar sir amet")
            .build()

        val animal: Animal = Animal.builder()
            .name("Toffee")
            .id("7253b404-a27a-4bdf-a6e4-25945b64ad54")
            .customer(customer)
            .build()

        val note: Note = Note.builder()
            .customer(customer)
            .animal(animal)
            .text("Note")
            .build()

        lifecycleScope.launch(Dispatchers.IO) {
            try {
                val response = Amplify.API.mutate(ModelMutation.create(note))
                if (response.hasData()) {
                    Log.i("MyAmplifyApp", "${response.data::class.simpleName} with id: ${response.data}")
                }
                if (response.hasErrors()) {
                    Log.i("MyAmplifyApp", "mutation failed: ${response.errors}")
                }
            } catch (error: ApiException) {
                Log.e("MyAmplifyApp", "Create failed", error)
            }
        }

Log output

``` // Put your logs below this line 2023-05-03 09:54:09.938 16927-16962 AndroidRuntime com.example.testimagecropper E FATAL EXCEPTION: DefaultDispatcher-worker-1 Process: com.example.testimagecropper, PID: 16927 java.lang.NullPointerException at java.util.Objects.requireNonNull(Objects.java:220) at com.amplifyframework.api.aws.AppSyncGraphQLRequestFactory.getMapOfFieldNameAndValues(AppSyncGraphQLRequestFactory.java:384) at com.amplifyframework.api.aws.AppSyncGraphQLRequestFactory.buildMutation(AppSyncGraphQLRequestFactory.java:211) at com.amplifyframework.api.graphql.model.ModelMutation.create(ModelMutation.java:41) at com.example.testimagecropper.MainActivity$onCreate$1.invokeSuspend(MainActivity.kt:97) at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106) at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42) at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95) at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664) Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@9b0fe6a, Dispatchers.IO] ```

GraphQL Schema

```graphql // Put your schema below this line type Measurement @model @auth(rules: [{allow: public}]) { id: ID! value: Int Notes: [Note] @hasMany(indexName: "byMeasurement", fields: ["id"]) customer_id: ID! @index(name: "byCustomer") animal_id: ID @index(name: "byAnimal") Animal: Animal @belongsTo(fields: ["animal_id"]) Customer: Customer @belongsTo(fields: ["customer_id"]) } type Customer @model @auth(rules: [{allow: public}]) { id: ID! name: String Measurements: [Measurement] @hasMany(indexName: "byCustomer", fields: ["id"]) Notes: [Note] @hasMany(indexName: "byCustomer", fields: ["id"]) Animals: [Animal] @hasMany(indexName: "byCustomer", fields: ["id"]) } type Note @model @auth(rules: [{allow: public}]) { id: ID! text: String measurement_id: ID @index(name: "byMeasurement") customer_id: ID! @index(name: "byCustomer") animal_id: ID @index(name: "byAnimal") Animal: Animal @belongsTo(fields: ["animal_id"]) Measurement: Measurement @belongsTo(fields: ["measurement_id"]) Customer: Customer @belongsTo(fields: ["customer_id"]) } type Animal @model @auth(rules: [{allow: public}]) { id: ID! name: String customer_id: ID! @index(name: "byCustomer") Measurements: [Measurement] @hasMany(indexName: "byAnimal", fields: ["id"]) Notes: [Note] @hasMany(indexName: "byAnimal", fields: ["id"]) Customer: Customer @belongsTo(fields: ["customer_id"]) } ```
mattcreaser commented 1 year ago

Thanks for the report @shri-onecup. We are looking into this.

mattcreaser commented 1 year ago

The same insertion works fine using DataStore:

    Amplify.DataStore.save(customer)
    Amplify.DataStore.save(animal)
    Amplify.DataStore.save(note)

DataStore uses a different request factory class that simply ignores the null field value here: https://github.com/aws-amplify/amplify-android/blob/main/aws-datastore/src/main/java/com/amplifyframework/datastore/appsync/AppSyncRequestFactory.java#L452

It looks like the change to AppSyncGraphQLRequestFactory to require a non-null association was introduced way back in version 1.6.4 with this commit: https://github.com/aws-amplify/amplify-android/commit/da6df078ebd32bf292d640858e1fd7c949f9d5f1

I have made a fix to AppSyncGraphQLRequestFactory that addresses this issue and allows the create snippet from the issue description to succeed. I'll continue to test the change through the day and will hopefully be able to open a PR ahead of our next release.

srimalaya commented 1 year ago

@mattcreaser thank you for the prompt update and fix. I’m looking forward to it being released soon.

eeatonaws commented 1 year ago

A fix has been released. Please update to Amplify Android 2.8.4 and let us know if you have any additional questions.

github-actions[bot] commented 1 year ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.