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

Issue with camel case endpoints #457

Closed
rayko opened this issue Jun 14, 2016 · 2 comments
Closed

Issue with camel case endpoints #457

rayko opened this issue Jun 14, 2016 · 2 comments

Comments

@rayko
Copy link
Contributor

rayko commented Jun 14, 2016

When using camel case on endpoint names, the app throws an error while loading the grape clases and modules:

/home/rayko/.rvm/gems/ruby-2.2.5@app/gems/grape-swagger-0.20.3/lib/grape-swagger.rb:90:in `block in combine_namespace_routes': undefined method `reject' for nil:NilClass (NoMethodError)

The following method in grape-swagger.rb

      def combine_routes(app, doc_klass)
        app.routes.each do |route|
          route_path = route.path
          route_match = route_path.split(/^.*?#{route.prefix.to_s}/).last
          next unless route_match
          route_match = route_match.match('\/([\w|-]*?)[\.\/\(]') || route_match.match('\/([\w|-]*)$')
          next unless route_match
          resource = route_match.captures.first
          next if resource.empty?
          resource.downcase!
          @target_class.combined_routes[resource] ||= []
          next if doc_klass.hide_documentation_path && route.path.match(/#{doc_klass.mount_path}($|\/|\(\.)/)
          @target_class.combined_routes[resource] << route
        end
      end

Causes to change the name that is picked up in the next step on the method:

      def combine_namespace_routes(namespaces)
        # iterate over each single namespace
        namespaces.each do |name, namespace|
          # get the parent route for the namespace
          parent_route_name = name.match(%r{^/?([^/]*).*$})[1]
          parent_route = @target_class.combined_routes[parent_route_name]
          # fetch all routes that are within the current namespace
          namespace_routes = parent_route.reject do |route|
            !route_path_start_with?(route, name) || !route_instance_variable_equals?(route, name)
          end

          if namespace.options.key?(:swagger) && namespace.options[:swagger][:nested] == false
            # Namespace shall appear as standalone resource, use specified name or use normalized path as name
            identifier =  if namespace.options[:swagger].key?(:name)
                            name.tr(' ', '-')
                          else
                            name.tr('_', '-').gsub(/\//, '_')
                          end
            @target_class.combined_namespace_identifiers[identifier] = name
            @target_class.combined_namespace_routes[identifier] = namespace_routes

            # # get all nested namespaces below the current namespace
            sub_namespaces = standalone_sub_namespaces(name, namespaces)
            sub_routes = sub_routes_from(parent_route, sub_namespaces)
            @target_class.combined_namespace_routes[identifier].push(*sub_routes)
          else
            # default case when not explicitly specified or nested == true
            standalone_namespaces = namespaces.reject do |_, ns|
              !ns.options.key?(:swagger) ||
                !ns.options[:swagger].key?(:nested) ||
                ns.options[:swagger][:nested] != false
            end

            parent_standalone_namespaces = standalone_namespaces.reject { |ns_name, _| !name.start_with?(ns_name) }
            # add only to the main route if the namespace is not within any other namespace appearing as standalone resource
            if parent_standalone_namespaces.empty?
              # default option, append namespace methods to parent route
              @target_class.combined_namespace_routes[parent_route_name] = [] unless @target_class.combined_namespace_routes.key?(parent_route_name)
              @target_class.combined_namespace_routes[parent_route_name].push(*namespace_routes)
            end
          end
        end
      end

If you have an endpoint like "userCompanies", it gets mapped to "usercompanies" in combine_routes, and combine_namespace_routes will expect @target_class.combined_routes to have a key "userCompanies", but since it doesn't exist, it will return nil and reject will be executed on nil.

There was a PR about this and it was merged: https://github.com/ruby-grape/grape-swagger/pull/321/files but the change is not in master or the 0.21.0 version. I wonder what happened to it. It looks like the PR was done on an old version, since the method combine_routes isn't present there, I suppose it was extracted as a method on a refactor after that.

What happened to this fix?

@dblock
Copy link
Member

dblock commented Jun 15, 2016

The HEAD version was to support swagger 2.0, I suspect this got lost in the shuffle. You should write a spec and re-apply the fix in a PR, please, also cc: @gekola who made the fix you mention above.

rayko added a commit to rayko/grape-swagger that referenced this issue Jun 15, 2016
@rayko
Copy link
Contributor Author

rayko commented Jun 15, 2016

There you go, I'll notify the other author about it.

dblock pushed a commit that referenced this issue Jun 15, 2016
* Fixed camel case issue #457

* Description of fixed issue in CHANGELOG.md

* Fixing api_swagger_v2_detail_spec.rb with updated expectation
@LeFnord LeFnord closed this as completed Jun 15, 2016
LeFnord pushed a commit to LeFnord/grape-swagger that referenced this issue Feb 9, 2019
* Fixed camel case issue ruby-grape#457

* Description of fixed issue in CHANGELOG.md

* Fixing api_swagger_v2_detail_spec.rb with updated expectation
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

3 participants