JsonApiClient / json_api_client

Build client libraries compliant with specification defined by jsonapi.org
MIT License
362 stars 186 forks source link

`belongs_to` combined with `shallow_path` breaks request url #369

Open wpliao1989 opened 4 years ago

wpliao1989 commented 4 years ago

Steps to reproduce:

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem 'json_api_client', '1.8.0'
end

class Resource < JsonApiClient::Resource
  self.site = 'http://example.com/api'
end

class Author < Resource
end

class Store < Resource
end

class Book < Resource
  belongs_to :author, shallow_path: true
  belongs_to :store, shallow_path: true
end

require 'minitest/autorun'

class PathTest < MiniTest::Test
  def test_path
    assert_equal 'books', Book.path({}) # This breaks because the result is `/books` instead of `books`
  end

  def test_get
    Book.all # Sends requests to http://example.com/books instead of http://example.com/api/books
  end
end

What's wrong:

This part of code: https://github.com/JsonApiClient/json_api_client/blob/db890adf3d7175e91829d359336ab7c98ed90a34/lib/json_api_client/resource.rb#L324

a.set_prefix_path(attrs, route_formatter) can return nil if using shallow_path. If there are 2 or more belongs_to association in the resource, paths.join("/") would return extra slashes ([nil, nil].join('/') => '/').

Possible fix:

Remove nils:

def _set_prefix_path(attrs)
  paths = _belongs_to_associations.map do |a|
    a.set_prefix_path(attrs, route_formatter)
  end

  paths.compact.join("/")
end

Also, I'm not sure what _prefix_path is meant to do so maybe we need to fix that method too.

ollizeyen commented 3 years ago

Just opened a PR which addresses this issue. Multiple belongs_to :resource, shallow_path: true will now be possible.

wpliao1989 commented 3 years ago

Thanks! Did you look into _prefix_path method on line 311 as well? Its implementation is very similar to _set_prefix_path.

ollizeyen commented 3 years ago

Just checked again. Compacted the array in the path getter as well!

ollizeyen commented 3 years ago

Will add tests early tomorrow. At least the current suite doesn't break locally. Is there any difference on travis? On travis the runner fails.