[openstreetmap/openstreetmap-website] Refactor usage of check_api_readable/writable (Issue #4858)

[Andy Allan]

The review of #4605 made me look more closely at the usage of `check_api_readable` and `check_api_writable` in existing controllers, and I realised that it can be a bit confusing.

The two helpers return true or false depending on `api_status`, which itself returns one of three states.

|`api_status`|`check_api_readable`|`check_api_writable`|
|---|---|---|
|online|:heavy_check_mark: true|:heavy_check_mark: true|
|readonly|:heavy_check_mark: true|:heavy_multiplication_x: false|
|offline|:heavy_multiplication_x: false|:heavy_multiplication_x: false|

Note that it's not possible (nor is it logical) for the api to be `_writable` but not `_readable`.

---

There are therefore two approaches that we could standardise on, either:
* Every method that needs some kind of API access either checks `_readable` or `_writable`, but not both
* Every method that needs write access checks both (read actions only check `_readable`, of course).

The first approach seems logical, but leads to long lists of `:except` exclusions e.g.
```ruby
    before_action :check_api_writable, :only => [:create, :update, :delete]
    before_action :check_api_readable, :except => [:create, :update, :delete]
```

The second approach leads to only maintaining one list of actions, e.g.
```ruby
    before_action :check_api_readable
    before_action :check_api_writable, :only => [:create, :update, :delete]
```

Technically this is redundant, since `_writable` also ensures that the api_status is in some kind of read mode. But I prefer this approach since it is easier to read and, I think, to maintain.

----
I also made a quick review of existing controllers, and it seems some are a bit of a mess:

```ruby
class ChangesetCommentsController < ApiController
    before_action :check_api_writable
    before_action :check_api_readable, :except => [:create]
```
This makes no sense, if all actions need write access then why would one action be excluded from the readable check?

```ruby
  class ChangesetsController < ApiController
    before_action :check_api_writable, :only => [:create, :update, :upload, :subscribe, :unsubscribe]
    before_action :check_api_readable, :except => [:index, :create, :update, :upload, :download, :subscribe, :unsubscribe]
```
Why does changesets#index and changesets#download not have any checks on the api_status being online/readonly? They aren't covered by either check.

I think both of these show the difficulties in trying to maintain `:except` lists.



-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/issues/4858
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/issues/4858 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240529/c3620994/attachment-0001.htm>