Update Android relationship docs to account for cascading delete #109
Conversation
| @@ -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"), | |||
There was a problem hiding this comment.
I'm kind of confused by this statement. Isn't "Author + Post deleted" more clear?
There was a problem hiding this comment.
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` model instance will be deleted and the `Author` model instance will be deleted, locally, and on the backend. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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. | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"?
| Amplify.DataStore.delete(article, | ||
| deletedArticle -> Log.i("Amplify DataStore", "Article deleted"), | ||
| failure -> {}); | ||
| Amplify.DataStore.query(Publication.class, Where.id("YOUR_PUBLICATION_ID"), |
There was a problem hiding this comment.
Just something to note: we don't yet have the fix for ambiguity introduced by Where.id() method...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
No adjustment was made to Where.id() yet. I haven't yet found a clean way to implement it.
| 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) }) | ||
| } |
There was a problem hiding this comment.
indentation feels awkward here. Should it be one more level deeper?
There was a problem hiding this comment.
I just updated the indentation for these two entire files!
Co-authored-by: Raphael Kim <[email protected]>
Co-authored-by: Raphael Kim <[email protected]>
Co-authored-by: Raphael Kim <[email protected]>
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.