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.

Sign up for GitHub

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

Update Android relationship docs to account for cascading delete #109

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Update Android relationship docs to account for cascading delete #109

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1afad90
Update java_relationship to account for cascading delete
richardmcclellan Feb 4, 2021
b027568
Update markdown/tests/android/java_relationship.md
richardmcclellan Feb 4, 2021
0c5b211
Update markdown/tests/android/kotlin_relationship.md
richardmcclellan Feb 4, 2021
769838c
Update markdown/tests/android/kotlin_relationship.md
richardmcclellan Feb 4, 2021
ab4d21f
Fix indentation
richardmcclellan Feb 4, 2021
98ed0f4
Simply success log message for deleting target model
richardmcclellan Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 22 additions & 22 deletions markdown/tests/android/java_relationship.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,22 @@ In one-to-one relationships, if the target model instance is deleted, it will al

```java
Amplify.DataStore.delete(author,
deleted -> Log.i("Amplify DataStore", "Author + Post deleted"),
deleted -> Log.i("Amplify DataStore", "Author deleted and relationship cleared on Post"),
Copy link

Choose a reason for hiding this comment

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

I'm kind of confused by this statement. Isn't "Author + Post deleted" more clear?

Copy link
Author

Choose a reason for hiding this comment

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

Well, the Post is not deleted. The Author belongs to the Post, not vice versa. I will remove the "and relationship cleared on Post" part, since that is basically implied.

failure -> Log.e("Amplify DataStore", "Deletion failed", failure));
```

In this example, the `Post`'s "author" field will be cleared and the `Author` model instance will be deleted.

If the source model is deleted, the target model will be automatically deleted as well.

```java
Amplify.DataStore.delete(post,
deleted -> Log.i("Amplify DataStore", "Author + Post deleted"),
failure -> Log.e("Amplify DataStore", "Deletion failed", failure));
```

In this example, the `Post` model instance will be deleted and the `Author` model instance will be deleted, locally, and on the backend.
Copy link

Choose a reason for hiding this comment

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

Doesn't Author have one-to-many relationship with Post? That means deleting post will not affect authors. Only if Author gets deleted, then will Post belonging to that specific Author will get deleted.

Copy link

Choose a reason for hiding this comment

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

Ooooh, I just looked at the original doc, and I think the document is wrong?

Example: One post (source model) has one author (target model).

Document describes that Post has-one Author, but the models are the other way around... This should probably be fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't Author have one-to-many relationship with Post?

I agree, realistically this would probably be a one-to-many, because an Author would never be restricted to just one Post. I'm actually struggling to think of a good use case for @HasOne at all. I think for now, we can stick with this example.

Document describes that Post has-one Author, but the models are the other way around... This should probably be fixed.

The question is - when deleting a Post, should it delete the Author? (this is how the document reads now) Or when deleting an Author, should it delete the Post (this is what you are proposing). I think either is valid, it just depends on the use case.

Copy link

Choose a reason for hiding this comment

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

Since Post has one Author, your edit is correct! :)

My only concern at the moment is that the other parts of the document (not edited in this PR) is currently incorrect.


:::NEW_COMMAND:::
:::ONE_TO_MANY:::

Expand Down Expand Up @@ -104,30 +114,20 @@ Amplify.DataStore.query(Article.class, Where.matches(Article.PUBLICATION_ID.eq("

**Delete**

In one-to-many relationships, delete the target model instance first and then delete the source model.
In one-to-many relationships, deleting the source model will automatically delete all target models that belong to it, both locally, and on the backend.
Copy link

Choose a reason for hiding this comment

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

I personally prefer "parent and child" over "source and target" because I think it's clearer, but I can see the value in keeping a consistent language across different platforms too. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I like "parent and child" better as well, but consistency across platforms is most important. @renebrandel what do you think about changing this wording across all platforms to "parent and child" instead of "source and target"?


```java
Amplify.DataStore.query(Article.class, Where.matches(Article.PUBLICATION_ID.eq("YOUR_PUBLICATION_ID")),
matches -> {
while (matches.hasNext()) {
Article article = matches.next();
Amplify.DataStore.delete(article,
deletedArticle -> Log.i("Amplify DataStore", "Article deleted"),
failure -> {});
Amplify.DataStore.query(Publication.class, Where.id("YOUR_PUBLICATION_ID"),
Copy link

Choose a reason for hiding this comment

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

Just something to note: we don't yet have the fix for ambiguity introduced by Where.id() method...

Copy link
Author

@richardmcclellan richardmcclellan Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, wasn't it fixed by aws-amplify/amplify-android#1133 to default to the class being queried? In this case, Publication, so this should actually work?

Copy link

Choose a reason for hiding this comment

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

No adjustment was made to Where.id() yet. I haven't yet found a clean way to implement it.

matchedPublications -> {
while(matchedPublications.hasNext()) {
richardmcclellan marked this conversation as resolved.
Show resolved Hide resolved
Publication match = matchedPublications.next();
Amplify.DataStore.delete(match,
deleted -> Log.i("Amplify DataStore", "Publication and all related Article instances deleted"),
failure -> Log.e("Amplify DataStore", "Deletion failed.", failure));
}
Amplify.DataStore.query(Publication.class, Where.id("YOUR_PUBLICATION_ID"),
matchedPublications -> {
while(matchedPublications.hasNext()) {
Publication match = matchedPublications.next();
Amplify.DataStore.delete(match,
deleted -> Log.i("Amplify DataStore", "Publication deleted"),
failure -> Log.e("Amplify DataStore", "Deletion failed.", failure));
}
},
failure -> {});

}, failure -> {}
);
},
failure -> {});

```

In this example, the `Publication` with "YOUR_PUBLICATION_ID" and all of its related `Article` model instances will be deleted.
Expand Down
42 changes: 21 additions & 21 deletions markdown/tests/android/kotlin_relationship.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,22 @@ In one-to-one relationships, if the target model instance is deleted, it will al

```kt
Amplify.DataStore.delete(newAuthor,
{ Log.i("Amplify DataStore", "Author + Post deleted") },
{ Log.i("Amplify DataStore", "Author deleted and relationship cleared on Post") },
{ failure -> Log.e("Amplify DataStore", "Deletion failed", failure) })
```

In this example, the `Post`'s "author" field will be cleared and the `Author` model instance will be deleted.

If the source model is deleted, the target model will be automatically deleted as well.

```kt
Amplify.DataStore.delete(post,
{ Log.i("Amplify DataStore", "Author + Post deleted") },
{ failure -> Log.e("Amplify DataStore", "Deletion failed", failure) })
richardmcclellan marked this conversation as resolved.
Show resolved Hide resolved
```

In this example, the `Post` model instance will be deleted and the `Author` model instance will be deleted, locally, and on the backend.

:::NEW_COMMAND:::
:::ONE_TO_MANY:::

Expand Down Expand Up @@ -102,29 +112,19 @@ Amplify.DataStore.query(Article::class.java, Where.matches(Article.PUBLICATION_I

**Delete**

In one-to-many relationships, delete the target model instance first and then delete the source model.
In one-to-many relationships, deleting the source model will automatically delete all target models that belong to it, both locally, and on the backend.
richardmcclellan marked this conversation as resolved.
Show resolved Hide resolved

```kt
Amplify.DataStore.query(Article::class.java, Where.matches(Article.PUBLICATION_ID.eq("YOUR_PUBLICATION_ID")),
{ matches ->
while (matches.hasNext()) {
val article = matches.next()
Amplify.DataStore.delete(article!!,
{ deletedArticle -> Log.i("Amplify DataStore", "Article deleted") },
{ failure -> Log.e("Amplify DataStore", "Deletion failed.", failure)})
}
Amplify.DataStore.query(Publication::class.java, Where.id("YOUR_PUBLICATION_ID"),
{ matchedPublications ->
while (matchedPublications.hasNext()) {
val match: Publication = matchedPublications.next()
Amplify.DataStore.delete(match,
{ deleted -> Log.i("Amplify DataStore", "Publication deleted") },
{ failure -> Log.e("Amplify DataStore", "Deletion failed.", failure) })
}
}
) { failure -> Log.e("Amplify DataStore", "Query failed.", failure) }
Amplify.DataStore.query(Publication::class.java, Where.id("YOUR_PUBLICATION_ID"),
{ matchedPublications ->
while (matchedPublications.hasNext()) {
val match: Publication = matchedPublications.next()
Amplify.DataStore.delete(match,
{ deleted -> Log.i("Amplify DataStore", "Publication and all related Article instances deleted") },
{ failure -> Log.e("Amplify DataStore", "Deletion failed.", failure) })
}
Copy link

Choose a reason for hiding this comment

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

indentation feels awkward here. Should it be one more level deeper?

Copy link
Author

Choose a reason for hiding this comment

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

I just updated the indentation for these two entire files!

}
) { failure -> Log.e("Amplify DataStore", "Query failed.", failure)}
) { failure -> Log.e("Amplify DataStore", "Query failed.", failure) }
```

In this example, the `Publication` with "YOUR_PUBLICATION_ID" and all of its related `Article` model instances will be deleted.
Expand Down