Support all source types #1
Conversation
| resources_list = [] | ||
| nodes.each{|n| resources_list << n.name.to_sym} | ||
| return resources_list | ||
| @resources ||= Nokogiri::XML(@resources_res.get.body).xpath('/prestashop/api/*').collect { |resource| resource.name.to_sym } |
There was a problem hiding this comment.
This was mostly merged into one line to make it more clear that no get or parsing is done if @resoures is already set.
| stub_request(:any, @url_regex).to_return body: xml | ||
| end | ||
|
|
||
| def test_get_resources |
There was a problem hiding this comment.
I now see that this works differently than the original documentation, but matches the functionality of the code before my pull request. I can revert the test changes and fix the code if this preferable, let me know.
| xml_product = @resources_res["products/#{id}"].get.body | ||
| product = Product.xml2hash xml_product | ||
| products << product | ||
| @resources ||= Nokogiri::XML(@resources_res.get.body).xpath('/prestashop/api/*').collect { |resource| resource.name.to_sym } |
There was a problem hiding this comment.
This was mostly merged into one line to make it more clear that no get or parsing is done if @resoures is already set.
There was a problem hiding this comment.
Problem with memoization of @resources is that permissions assigned to an API key can be changed dynamically from the Prestashop administration. If an Api instance remembers the @resources array forever, this makes it necessary to get a new instance of the class after a change in permissions. I'd prefer if the list of resources accessible by the API key was not memoized, so that clients can get an up to date list of accessible resources at any time.
This would introduce a performance issue, however. You're using the @resources list in method_missing to see if the method being invoked matches an accessible resource. In this case we do want to get the memoized list of resources, so we don't make an extra HTTP call every time. Perhaps it would be best to create a new public get_resources method, that does an HTTP request every time, and make the resources method (that invokes get_resources only if @resources still has no value) private.
amatriain
left a comment
amatriain
left a comment
There was a problem hiding this comment.
This is really good stuff. Thanks for your interest! Also sorry for the delay in responding but I wanted to review it thoroughly.
I made some comments, could you take a look at them? Also some yard docs for the new methods would be nice.
| def method_missing(method, *args, &block) | ||
| if method.to_s.match(/^get_/) | ||
| resource = method.to_s.sub(/^get_/,'').pluralize.to_sym | ||
| raise RestClient::MethodNotAllowed, 'You do not have access to this resource' unless resources.include?(resource) |
There was a problem hiding this comment.
I don't like the gem raising RestClient errors. The fact that the gem uses RestClient to make HTTP calls should be an implementation detail transparent to an app that uses it.
On the other hand creating a hierarchy of errors only for this gem is overkill. I'm unsure about the best way to do it.
| ids.uniq.sort.collect { |id| get_resource(resource, id) }.compact | ||
| end | ||
|
|
||
| def collect_ids_for_resource(args) |
There was a problem hiding this comment.
Perhaps it would be better to call this method collect_id_for_resource, seeing that it validates and returns the ID of a single resource being requested :)
| Nokogiri::XML(@resources_res[resource].get.body).xpath("/prestashop/#{resource}/*/@id").collect(&:value) | ||
| end | ||
|
|
||
| def get_resource(resource, id, raise_not_found_exception = false) |
There was a problem hiding this comment.
What use is the raise_not_found_exception argument? The method is always invoked passing a true to this argument. Also there is no equivalent argument in get_resources method.
|
|
||
| begin | ||
| response = Nokogiri::XML(@resources_res[resource][id].get.body).remove_namespaces!.xpath("/prestashop/#{resource.to_s.singularize}") | ||
| JSON.parse(Hash.from_xml(response.to_s).values.first.to_json, object_class: resource_class) |
There was a problem hiding this comment.
Isn't a require 'active_support/json' necessary? Otherwise I think we'll get an undefined method error trying to invoke Hash.from_xml(response.to_s).values.first.to_json
| end | ||
|
|
||
| def get_resources(resource, ids) | ||
| ids.uniq.sort.collect { |id| get_resource(resource, id) }.compact |
There was a problem hiding this comment.
Is the sort step really necessary?
2 participants
I added to this pull request a number of times while developing with the gem. I believe that this very closely matches the current documentation while keeping all existing functionality and adding new functionality.