Fall back to authorizing update? on related records

[valscion]

This was the functionality present before v1.0.0 and should be kept intact. It is what we want to do — changes to relationships can be thought as changing the related records.

It seems that at some point, this functionality got accidentally lost. The change seems to date to #51

Seems like the reason this slipped through the cracks was that we didn't remember to check these cases back when relationship operation authorization was implemented in #40, so when #51 used the same methods, the old functionality accidentally got lost.

[valscion]

I'm not really sure what we should authorize in the case when a has-one relationship gets unset. To keep parity with adding a has-one relationship, we should authorize for the other side of the associated record when removing it.

Here's an example scenario set-up:

user_1 = User.create(id: 'user-1')
article_1 = Article.create(id: 'a-1', author: user_1)

DELETE /articles/a-1/relationships/author

(empty body)

or

PATCH /articles/a-1

{
  "type": "articles",
  "id": "a-1",
  "relationships": {
    "author": {
      "data": null
    }
  }
}

should both call:

• ArticlePolicy.new(current_user, article_1).remove_author?(user_1)
• or fall back to UserPolicy.new(current_user, user_1).update?

---

This is, because the reverse way, adding the author, goes like this:

article_2 = Article.create(id: 'a-2')
user_2 = User.create(id: 'user-2')

PATCH /articles/a-2/relationships/author

{
  "type": "users",
  "id": "user-2"
}

or

PATCH /articles/a-2

{
  "type": "articles",
  "id": "a-2",
  "relationships": {
    "author": {
      "data": {
        "type": "users",
        "id": "user-2"
      }
    }
  }
}

...and authorization goes like this for the relationship:

• ArticlePolicy.new(current_user, article_2).replace_author?(user_2)
• or fall back to UserPolicy.new(current_user, user_2).update?

[valscion]

Also, the above scenario makes me wonder whether it is a good idea to add the remove_<relationship_type>? method instead of calling replace_<relationship_type>? with nil when a has-one relationship gets unset.

Maybe we could do with just the replace_<singular_relationship>? method for all the cases touching a has-one relationship? Any thoughts, @matthias-g?

[valscion]

I think we've now got the same fallback behavior that was part of v0.8.2. The old TODO comment about replace_fields probably needing old records as well still stands, and that could be figured out after this PR is done

[valscion]

I'm gonna ship this as the next alpha version right away. If anyone would like to check out the code and review whether this looks good, I'd appreciate any comments even after this has been merged ☺️

[matthias-g]

Good catch! Thanks for adding this missing fallback.

Maybe we could do with just the replace_<singular_relationship>? method for all the cases touching a has-one relationship?

I think this is a matter of taste. I prefer having the two methods because if you have a scenario where the policies for replace_<something> and remove_<something> are different the policy looks cleaner if there are two separate methods. And when implementing a policy you don't have to remember to check for the argument passed to replace_<something> being nil.

The old TODO comment about replace_fields probably needing old records as well still stands, and that could be figured out after this PR is done

Yeah, makes sense. I think it was deleted not having the fallback to update? in mind.

[valscion]

I think this is a matter of taste. I prefer having the two methods because if you have a scenario where the policies for replace_<something> and remove_<something> are different the policy looks cleaner if there are two separate methods. And when implementing a policy you don't have to remember to check for the argument passed to replace_<something> being nil.

True.

However, this forces users to define two methods instead of just one. It could cause a weird place where first assigning a value to a relationship would be OK (considering replace_<something>? has returned a truthy value) but removing it later would not be OK (considering remove_<something>? had never been defined).

I don't really know which approach is better for the library user. All I know that if we'd be OK with sending nil to replace_<something>?, the gem code wouldn't have to have a special case for that in authorizer class and processor.

As I don't have a strong opinion either way, I'm fine with leaving it as it is now, i.e. keep both of the two methods.

We could even provide some validator for the policies as a means of increasing developer experience: We could warn if some custom method isn't defined for a relationship, e.g. if there would be a ArticlePolicy#replace_author? method defined but ArticlePolicy#remove_author? would not.