Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

belongs_to combined with shallow_path breaks request url #369

Open
wpliao1989 opened this issue Jul 15, 2020 · 4 comments
Open

belongs_to combined with shallow_path breaks request url #369

wpliao1989 opened this issue Jul 15, 2020 · 4 comments

Comments

@wpliao1989
Copy link

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:

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
Copy link

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

@wpliao1989
Copy link
Author

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

@ollizeyen
Copy link

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

@ollizeyen
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants