remi
No way to handle 404s?
Is there a way to handle 404 status codes when an object was not found on the other side?
Say that we have a model Post and we have set things up so that we can call Post.find(1) and there is not Post object with id=1 on the other side. Let's say the other API returns nil and status code 404. Her doesn't seem to consider the status code and rather complains with an error about the nil. What is the right way to handle such situations?
Reopening this as custom paths don't seem to handle 404s well after all. Even when 404 is returned Her prepares a model object but it has not attributes. I would expect it to return nil.
I notice the same thing, I would expect that Her returns nil in case of 40X HTTP codes returns.
i agree that returning nil makes more sense than an object with no attributes.
the place this would be handled is in the Her::Middleware layer. when parsing the object, it's not accounting for 4xx codes as you noticed, and should.
i'm doing some work at this level right now to handle json api compliant apis, so can take a look at this as well, or if you want to take a crack at PRing this yourself, that would be much appreciated.
thanks for the report.
the problem with returning nil is you have nothing to work with in the event of an error - if this is limited to 404's return nil, that might work
but sometimes it's useful to see what was returned, the http status code
ideally, all Her objects would have some simple way to see the http details like maybe a #_status that gives the code for the call, or maybe even a hash of relevant details {http_status_code: '404', http_method: 'GET'}
one of the biggest knocks I hear against Her is the lack of transparency when something goes wrong
I agree with ak47, sometimes you want to make decisions based on the status code. Like a 401 vs 422 vs 204.
I am working on something right now where on a DELETE of a resource I get a 204 for a successful delete and a 422 for a unsuccessful delete. Now, I have no way of knowing whether or not the call was successful or not on the client side.
Like ak47 said just having a hash of the details would relieve a lot of troubles I have with her.
i agree there needs to be a way to distinguish between things working or not. however, i don't agree that http status codes are the way to do that.
the her middleware layer is there to provide the adapter layer between the http details on the active model object that is returned.
for a 404 specifically, nil is the most obvious, but the tradeoff would be that we wouldn't be able to include any additional info the server provides. the other possibilities would be a "NullModel" of some sort or making find raise an exception akin to how ActiveRecord does it.
for a 422 or other 4xx responses, the way to determine whether or not the delete was success would be to query the errors object attached to the model. Her should probably add a generic error for 4xx cases and then add errors included by the server.
object.response_errors => [{:id=>"bb78d697-7b3b-4780-8b45-3c0eadaabf95", :status=>404, :title=>"Not found", :detail=> "thing not found by key=10a228f9-4dff-4f7d-8aba-1c88fd9a29d8", :at=>"2015-06-09T22:54:25.943Z"}]
I think @hubert is right here. System failure (including 401, 403, 404 and 5xx) should raise an exception. In those cases we can't rely on the API response to be sane. Content failure (e.g. 422 because invalid) should change model properties by passing through API error messages.
I would suggest a small family of exceptions that follow the usual status code names (Her::Errors::NotFound, Her::Errors::Unauthorized, Her::Errors::ServerError) and all inherit from Her::Errors::ResponseError.
Associates would sometimes need rescuing from fetch errors. Perhaps that should be configurable:
belongs_to :country, foreign_key: :country_code, on_error: :raise
For the model errors could I also suggest that we adopt ActiveModel::Errors to try and marshal the received errors hash or array into a standard format? As usual people would need a custom parser if their error format is weird, but the error-processing could be exposed as a separate method.
Custom exceptions are good but theye're not for everyone.
Maybe store object.response_status at least?
@marshall-lee yes, that would make sense for a processing error. The exceptions are meant for complete failures where we may not even have an object to store errors in. As @antonrd originally pointed out, creating a model object to communicate that there is no model object is too confusing.
To address the original issue report properly: I think the value of a missing object should be nil, and the action of fetching a missing object should raise an exception. Ideally this would be done in a way that is very easy to ignore. For single associations the default behaviour on 404 would be to nullify and continue, but for an explicit find or fetch call we would let the caller handle the exception just as ActiveRecord does.
I think the value of a missing object should be nil
You mean missing association objects? If yes I think the same.
and the action of fetching a missing object should raise an exception
As ActiveRecord throws ActiveRecord::RecordNotFound one would assume a similar behavior for Her. I definitely want this feature but I want it as optional for now. It shouldn't break existing code. For example in our project we handle errors (and not found errors too) by looking into response_errors. We don't like it but it's already implemented and it works, and we don't want our code be broken after upgrading to new version of Her. There should be a graceful upgrade path for existing users.
Agreed that behaving like ActiveRecord is always going to be comforting. In Her it's easy to configure response error behaviour while setting up an api because in the end all requests are made by Her::API#request. Nice work by Rémi there.
Her::API.setup url: "https://api.example.com", ignore_response_errors: [404]
The maintainer would have to decide about default behaviour after an upgrade but rails gives us a good example: warn and deprecate one version ahead, then change the default behaviour in a way that is easy to undo.
The maintainer would have to decide about default behaviour after an upgrade but rails gives us a good example: warn and deprecate one version ahead, then change the default behaviour in a way that is easy to undo.
Yeah, I agree.
@wil-r, @hubert I am interested in Her raising an error for unauthorized errors. Or somehow make that case visible.
Is anyone working on related behaviour?
If not I could help out with some guidance.
@alvinschur Here's my solution.
1 Create faraday middleware to parse JSON data
module ApiResponseHandler
class UnauthorizedError < StandardError; end
class CustomerParser < ::Faraday::Response::Middleware
def call(request_env)
@app.call(request_env).on_complete do |response_env|
json = MultiJson.load(response_env[:body], symbolize_keys: true)
case response_env[:status]
when 200
response_env[:body] = {
data: json,
errors: [],
metadata: {}
}
when 401
raise ApiResponseHandler::UnauthorizedError
end
end
end
end
end
2 Load custom parser instead of default one
# config/initializers/her.rb
Her::API.setup url: "https://api.example.com" do |c|
c.use ApiResponseHandler::CustomParser
end
3 When REST api returns 401 as http status, it raises ApiResponseHandler::UnauthorizedError
[1] pry(main)> User.find(1)
# raise FaradayApiResponseParser::UnauthorizedError
I hope this helps you.
Thank you for the detailed answer and example.
I am writing several libraries to access internal microservices.
I see a few options for including the error handling code
- duplicate the error handling code in each library created to access a microservice
- fork her and write the error handling code once
- contribute to her so others can benefit from the error handling code
Is it ok to include a middleware as described to handle the unauthorized error in her?
This error handling sounds quite similar to handling 404 and other errors.
Is it feasible to have one middleware handle a list of errors? Or do people prefer separate middleware, one per error: unauthorized, not found, server error, etc?
- a separate project similar to Faraday middleware for the extra middlewares we use
I don't know best practice how about her's error handling but I'm handling 40x and 50x errors in one middleware(extract gem) like above example code. If you have serval client application to use same REST API, I think your 4) solution is better.
Did this conversation go any further? Found myself bumping up against it today, expecting find(unknown id) to Raise an exception but it swallows it.
I am adding my solution here.
Our models do not include Her::Model directly but they all inherit from an ApplicationModel that includes Her::Model.
This gives us the possibility to override methods for all our models.
We did it for different things and one of these was to handle 404.
What we did is as simple as:
class ApplicationModel
include Her::Model
def self.find(params)
result = super
raise ActiveRecord::RecordNotFound unless result
result
end
# other stuff overrridden
end
I hope this helps somebody else
