Return errors as json objects instead of strings

[robbevp]

We currently have a mapping in config/locales/en.yml to convert error messages to keys, which we then look up in web's translations. I realised that by using @object.errors.errors we can just directly return the attributes of each error.

We now always return errors in the shape of:

{ "errors": [{ "model": "artist", "attribute": "name", "type": "blank" }] }

I opted to always return an object with the key errors and an array, to make have a consistent shape that we can easily distinguish from an unexpected error (or an error that we don't handle ourselves).

For the errors from our validations, we can easily converted these starting from the objects. In the future, we maybe add could options like: {"attribute": "title", "type": "too_long", "options": { "count": 50 } }
I ended up deciding against a map for the types here for two reasons:

• The default validations we currently used presence and uniqueness result in the types taken and blank which we were already using in web, so this would just become a hash that maps values to itself. This map would become rather big with all the custom types that we validate.
• I think the intention of a map was documenting which errors we might get, but I don't think we really accomplish this. A map of { taken: :taken } doesn't tell us which models and attributes could get this error. I would be great to document the potential errors in some way, but this isn't it

For the few "loose" errors, I converted these to the same shape and tried to keep the keys similar where this made sense. For the errors that start from a form with user input, I added attribute: :base to have an easy way of displaying errors in a form that aren't related to one specific field

• I've added tests relevant to my changes.

I added an extra assertion to all cases where we already tested for a response of unprocessable_content, not_found, forbidden, or unauthorized (unless I missed one)
(There were a few cases where we checked that an update return :unprocessable_content, but didn't actually verify that no changes were made. I added an extra assertion to those)

[chvp]

Did we ever talk about this in person? The one thing I find a bit icky about this is that this basically exposes the rails way of handling errors to the clients. IMO, it should always be theoretically possible to create another API without too much issues.

[robbevp]

Did we ever talk about this in person? The one thing I find a bit icky about this is that this basically exposes the rails way of handling errors to the clients. IMO, it should always be theoretically possible to create another API without too much issues.

IIRC: we said have some way of providing more structured errors to clients, but to add some layer to convert rails' structure to something else
Though when I now think about this, I'm not sure what that would be in practice. We could rename some keys and values but I don't think rails does a bad job of naming them.
Something like the following feels a bit superfluous to me:

ERROR_TYPE_MAP = {
  blank: :required,
  taken: :not_unique
}

def transform_error_for_json(error)
    result = { attribute: error.attribute, type: ERROR_TYPE_MAP[error.type] }
    result[:options] = error.options.except(*ActiveModel::Error::CALLBACKS_OPTIONS, *ActiveModel::Error::MESSAGE_OPTIONS)
    result
  end

Maybe we should just be more explicit about which other options we include? (Rather than including everything except some specific things).

def transform_error_for_json(error)
    result = %i[attribute type].index_with { |it| error.send(it) }
    result[:options] = error.options.slice(:count, :other_option)
    result
  end

[chvp]

Maybe we should just be more explicit about which other options we include? (Rather than including everything except some specific things).

This options seems fine to me.

[robbevp]

@chvp I finally finished this up. I reworked this PR based on the discussion above and updated the description to reflect the actual state of changes

I just noticed that the patch coverage is rather low, since I didn't add new tests if we didn't already check the response. I can still add those (though I think that there are a few cases where we could never actually get this situation - for example I don't think destroying an album can actually fail)

I'll leave this as a draft until we have a matching PRs in api-client-js and web. Do you think this needs any changes in the android app? (it doesn't for the iOS/macOS app)

[robbevp]

I had to make this change since I moved the the separator validation. I realised that there is an issue with this merge logic: some of these might silently fail. I'll create a separate issue for this

[robbevp]

Made a few more changes:

• Added the model key to the errors we render
• Removed the error type map (see above for the reasoning)
• Converted some of our custom error types so that they work with the new system

[chvp]

Not sure I agree with the policy key here. To me it feels like something we can still do with model, and the concept of a policy feels pundit-specific to me.

[chvp]

If we're skipping validations anyway, can we do update_all?