views: PIDConverter prevents from sending additional data during a GET of a deleted record

[nharraud]

Problem: In B2SHARE we would like to return the reason why a record was deleted when a client makes a GET request on a deleted request. This is currently not possible as the PIDConverter will raise a PIDDeletedRESTError exception which bypasses the serializer.

I don't have yet any clear solution for this problem. This is not an urgent issue.

[hachreak]

Do you think about use a different PIDConverter overwriting it in RECORDS_REST_ENDPOINTS configuration? Something like item_route='/records/<mypidconverter(recid):pid_value>',

[nharraud]

That would be the most backward compatible solution yes. But do we deprecate the previous one? The current behavior is quite limiting.

[lnielsen]

Records-UI already have a solution so it should be aligned with that.

[lnielsen]

Most likely we should add an error handler on the blueprint similar to the way it is done in Invenio-Records-UI

[slint]

@lnielsen @nharraud Apart from implementing a similar method as the one in invenio-records-ui, part of the flexibility that the particular error handler has, is the fact that the raised exception is an subclass of invenio_pidstore.errors.ResolverError (or its subclasses), which also contains some extra information (eg. the deleted record in case of a PIDDeletedError).

As part of adding the extra handlers, maybe it would be good to include the original PIDStore exception in the re-raised exception:

try:
    return resolver.resolve(pid_value)
except PIDDeletedError as exc:
    raise PIDDeletedRESTError(original_error=exc)

# ...or...

except PIDDeletedError as exc:
    from future.utils import raise_from
    raise_from(PIDDeletedRESTError(), exc)
    # Equivalent to Python 3:
    # raise PIDDeletedRESTError() from exc

# ...then in the handler:
@blueprint.errorhandler(PIDDeletedRESTError)
def err_handler(error):
    return jsonify({
        'status': error.code,
        'message': error.descriptionm
        'removal_reason': error.original_error.record.get('removal_reason'),
        # or if `from_raise` was used:
        # 'removal_reason': error.__cause__.record.get('removal_reason'),
    })

[lnielsen]

@slint Yes, actually the current way of catching and rasing a new exception is likely not very good, so as minimum we should reraise with same stacktrace and the included data.

[lnielsen]

Now the problem is that we likely need a serliazer or simiarl to customize what is returned in the tombstone.

[slint]

I was thinking of a [ExceptionClass -> "path.to.handler.function"] dictionary in config, which will be registered using blueprint.register_error_handler as part of the create_blueprint function... The handler function could be something more elaborate that uses a schema to serialize the response

[nharraud]

Could we have the handler in the RECORDS_REST_ENDPOINTS config rather than a separate config variable? That way we could have a separate handler per endpoint. There can also be a default handler in a separate config variable.

Can we suppose that there will be only one format returned in case of error. Example: request "accept-type:application/marcxml" -> error -> return JSON I think that it can make sense otherwise you would have conflict between 406 (not-acceptable) and 404/409/...