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

Links broken when used with scope module: routes #591

Open
mitnal opened this issue Jan 18, 2016 · 17 comments
Open

Links broken when used with scope module: routes #591

mitnal opened this issue Jan 18, 2016 · 17 comments

Comments

@mitnal
Copy link

mitnal commented Jan 18, 2016

Hi,

we use a version parameter inside of the Accept header to be able to switch Api versions. The different versions are grouped in modules. And to remove these modules as namespaces from the generated URLs we use scope module:. This works great but the problem ist that the generated links inside a response still include the module.

The code just checks the module hierarchy of the resource class and completely ignores how rails generates the routes. (See: https://github.com/cerebris/jsonapi-resources/blob/master/lib/jsonapi/link_builder.rb#L102).
Also see #361 for a similar problem.

As a workaround we monkey patch the formatted_module_path_from_class method to return the correct path \api\. But that is not an ideal solution.

An example route file:

namespace :api do
  scope module: :v1, constraint: ApiConstraint.new(version: 1) do
    jsonapi_resources :my_resource
  end
end

And an example resource:

module Api
  module V1
    class MyResource < JSONAPI::Resource
      [...]
    end
  end
end

So Rails generates routes like: /api/my-resource
But the generated links look like this: /api/v1/my-resource

Sascha

@ghost
Copy link

ghost commented Jan 20, 2016

👍

I have this issue as well, except I use my version in the url instead of the 'api' namespace.

routes.rb

constraints(subdomain: "api") do
  scope module: "api", defaults: { format: :json } do
    namespace :v1 do
      jsonapi_resources :news
    end
  end
end

In the JSON, I'm getting links like:

self: "http://api.iu.com:5000/api/v1/news/1"

When they should look like:

self: "http://api.iu.com:5000/v1/news/1"

@coryondemand
Copy link

Also related to:

#257

It would be nice to have a solution to handle this as we also version our API via headers and do not include the version in the path.

@beechnut
Copy link
Contributor

👍

@mitnal: Could you post the code for the monkey patch? Like you say, it's not ideal, but it's better than nothing at the moment.

Thanks in advance!

@beechnut
Copy link
Contributor

Borrowing from the gem's LinkBuilder tests, I added the following hacks to my app, for the moment.
With this, my links are pointing to the right place.

I'm matching the slash at the end because doing the one at the beginning would catch the '/api' in http://api.example.com.

# test/lib/jsonapi_test.rb

require 'test_helper'

class JSONAPITest < ActiveSupport::TestCase

  def setup
    @base_url        = "http://example.com"
    @route_formatter = JSONAPI.configuration.route_formatter
    @search          = searches(:exact) # Grabs one of my fixtures
  end

test "route without /api in path" do
    primary_resource_klass = API::SearchResource

    config = {
      base_url: @base_url,
      route_formatter: @route_formatter,
      primary_resource_klass: primary_resource_klass,
    }

    builder = JSONAPI::LinkBuilder.new(config)
    source  = primary_resource_klass.new(@search, nil)
    expected_link = "#{ @base_url }/searches/#{ source.id }"

    assert_equal expected_link, builder.self_link(source)
  end
end
# lib/extensions/jsonapi.rb

module JSONAPI
  class LinkBuilder

    private

    def formatted_module_path_from_class(klass)
      scopes = module_scopes_from_class(klass)

      unless scopes.empty?
        # The Main Hack: adding the #gsub
        "/#{ scopes.map(&:underscore).join('/') }/".gsub(/api\//, '')
      else
        "/"
      end
    end

  end
end

@beechnut
Copy link
Contributor

Since I'm not super familiar with Rails' internals, I'm not entirely clear how we would go about solving this. @mitnal et al, do you know where this lives, and what we'd need to add to solve the general case here?

@mitnal
Copy link
Author

mitnal commented Jan 29, 2016

@beechnut: I have a very simple workaround and it just ignores klass all together 😄

module JSONAPI
  class LinkBuilder
    def formatted_module_path_from_class(klass)
      '/api/'
    end
  end
end

I just checked and it should be possible to create links via Rails but I do not know why we need this big LinkBuilder klass. I assume there is some hidden complexity.

What I testes was the following:

Routes:

namespace :api do
  scope module: :v1, constraint: ApiConstraint.new(version: 1) do
    jsonapi_resources :my_resource
  end
end

Resource:

module Api
  module V1
    class MyResource < JSONAPI::Resource
      model_name 'MyResourceModel'
      [...]
    end
  end
end

Rails Console:

app.url_for(Api::V1::MyResource.new(MyResourceModel.last, nil)._model)
# "http://www.example.com/api/my-resource/ffd9304a-036a-4b14-a871-8e2c339d5de2"

So it should be possible to somehow use Rails to create the links. Unfortunately I have no time at the moment and to be honest I do not know JSON API well enough to know all the requirements for resource linking.

Also there seems to be a lot of code that is dedicated to create links and I just looked at this one method.

@beechnut
Copy link
Contributor

Clever -- that's much simpler! Since I have both Accept header-based versioning and an 'api' subdomain, I changed mine to:

def formatted_module_path_from_class(klass)
  '/'
end

Also, @mitnal: a heads-up on your last comment: I think the url_for call should have a .new.

app.url_for(Api::V1::MyResource.new(MyResourceModel.last, nil)._model)

@mitnal
Copy link
Author

mitnal commented Jan 30, 2016

@beechnut thx, fixed my code snipped.

@davidgoli
Copy link

👍 thanks for the workarounds! but a fix would be splendid

@Fivell
Copy link

Fivell commented Feb 26, 2016

possible related #636

@ntippie
Copy link

ntippie commented May 7, 2016

Here's an alternate solution that removes a set of EXCLUDED_MODULES and also removes a version module by RegEx. These are unnecessary in my URLs because my Api namespace is constrained on the subdomain and the API version scope is constrained by the HTTP headers. Thanks to @beechnut for the starting point.

# lib/core_ext/jsonapi.rb

module JSONAPI
  class LinkBuilder

    EXCLUDED_MODULES = ['Api']
    EXCLUDED_VERSION_REGEX = /V\d{,2}/

    private

    def formatted_module_path_from_class(klass)
      scopes = module_scopes_from_class(klass)
      scopes -= EXCLUDED_MODULES
      scopes.reject! {|scope| scope=~ EXCLUDED_VERSION_REGEX}
      path = '/'
      unless scopes.empty?
        path += "#{ scopes.map(&:underscore).join('/') }/"
      end
      path
    end

  end
end
# lib/spec/jsonapi_spec.rb

require 'spec_helper'

describe 'JSONAPI::LinkBuilder extension' do
  let(:base_url) {'http://api.test.com'}
  let(:builder_klass) {JSONAPI::LinkBuilder}
  let(:route_formatter) {JSONAPI.configuration.route_formatter}

  describe '#formatted_module_path_from_class' do
    let(:primary_resource_klass) {Api::V1::UserResource}
    let(:user) {create(:user)}

    it 'should exclude API::V\d{,2} from path' do
      config = {
          base_url: base_url,
          route_formatter: route_formatter,
          primary_resource_klass: primary_resource_klass,
      }
      builder = builder_klass.new(config)
      source = primary_resource_klass.new(user, nil)
      expected_link = "#{ base_url }/users/#{ source.id }"
      expect(builder.self_link(source)).to eq(expected_link)
    end
  end
end

@jufemaiz
Copy link

I think that you're on the way using app.url_for, however I'm not sure it's perfect just yet. This is mainly due to the fact that the Rails Routes have a variety of options that can be used:

  • subdomain constraints
  • other custom constraints (e.g. ApiConstraints as given above to provide versioning constraints)
  • path: nil

Really we should be using ActionDispatch::Routing::UrlFor if we're relying on the Rails Routing engine for inbound routing and not extracting the routing to elsewhere.

As such, we do need to know the controller and action to use in order to determine the route, yes?

include UrlFor
url_for(controller: 'users',
        action: 'new',
        message: 'Welcome!',
        only_path: true)

@ntippie your solution looks like it will work but is highly configured and needs mapping tightly with the selected routing structure.

Reading through the commentary with Devise routing (rabbit warren of #380 » rails/rails#21231 » https://github.com/plataformatec/devise/pull/3714/files ) I'm now looking into how the Routing methods of JSONAPI::Resource are dealt with.

@peco8
Copy link

peco8 commented Oct 5, 2016

Hi I'm new to use jsonapi-resources. In production, should I avoid using subdomain? It hasn't solved yet.

@danwetherald
Copy link

Does anyone have a solution for this issue yet?

@danwetherald
Copy link

danwetherald commented Dec 22, 2016

This is what I ended coming up with to fix the issue of having /api/ in the links:

class LinkBuilder
    def formatted_module_path_from_class(klass)
      "/#{klass.to_s.split('::')[1].downcase}/"
    end
  end
end

@santostiago
Copy link

How do you include this monkey patch?
I tried adding require_relative '../lib/core_ext/json_api.rb' to the application.rbbut it didn't do anything.

philiprlarie added a commit to philiprlarie/recipes that referenced this issue Dec 8, 2018
@Albertc
Copy link

Albertc commented Dec 19, 2018

Hi, is there a solution without monkey-patching. As I understand the problem is the same I have:

If in routes I have scope module: 'site'
and in views form_with model: @family generates action="/site/families/14" but should generate action="/families/14"

Thanks

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

No branches or pull requests