From d8f10754919598af5bdb58c1f4a5dfafb655399c Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Mon, 27 Apr 2015 17:01:06 +0200 Subject: [PATCH 1/6] start working on repos by owner endpoint --- lib/travis/api/v3/access_control/anonymous.rb | 5 ++++ lib/travis/api/v3/access_control/generic.rb | 6 +++++ lib/travis/api/v3/access_control/user.rb | 4 ++++ lib/travis/api/v3/queries/repositories.rb | 23 +++++++++++-------- lib/travis/api/v3/routes.rb | 1 + .../api/v3/services/owner/repositories.rb | 10 ++++++++ 6 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 lib/travis/api/v3/services/owner/repositories.rb diff --git a/lib/travis/api/v3/access_control/anonymous.rb b/lib/travis/api/v3/access_control/anonymous.rb index f2229bc5..c41df37f 100644 --- a/lib/travis/api/v3/access_control/anonymous.rb +++ b/lib/travis/api/v3/access_control/anonymous.rb @@ -9,6 +9,11 @@ module Travis::API::V3 new end + def visible_repositories(list) + return [] unless unrestricted_api? + list.where(private: false) + end + def admin_for(repository) raise LoginRequired end diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index 8220d653..c4883ff3 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -30,6 +30,12 @@ module Travis::API::V3 false end + def visible_repositories(list) + # naïve implementation, replaced with smart implementation in specific subclasses + return list if full_access? + list.select { |r| visible?(r) } + end + protected def build_visible?(build) diff --git a/lib/travis/api/v3/access_control/user.rb b/lib/travis/api/v3/access_control/user.rb index 38529f8a..ff8eff44 100644 --- a/lib/travis/api/v3/access_control/user.rb +++ b/lib/travis/api/v3/access_control/user.rb @@ -19,6 +19,10 @@ module Travis::API::V3 permission?(:admin, repository) ? user : super end + def visible_repositories(list) + list.where('repositories.private = false OR repositories.id IN ?'.freeze, permissions.map(&:repository_id)) + end + protected def repository_writable?(repository) diff --git a/lib/travis/api/v3/queries/repositories.rb b/lib/travis/api/v3/queries/repositories.rb index 7872e8d4..61cf3a60 100644 --- a/lib/travis/api/v3/queries/repositories.rb +++ b/lib/travis/api/v3/queries/repositories.rb @@ -6,16 +6,21 @@ module Travis::API::V3 all.joins(:users).where(users: user_condition(user)) end + def for_owner(owner) + filter(owner.repositories) + end + def all - @all ||= begin - all = Models::Repository - all = all.where(active: bool(active)) unless active.nil? - all = all.where(private: bool(private)) unless private.nil? - all = all.includes(:owner) if includes? 'repository.owner'.freeze - all = all.includes(:last_build) if includes? 'repository.last_build'.freeze - all = all.includes(:default_branch) - all - end + @all ||= filter(Models::Repository) + end + + def filter(list) + list = list.where(active: bool(active)) unless active.nil? + list = list.where(private: bool(private)) unless private.nil? + list = list.includes(:owner) if includes? 'repository.owner'.freeze + list = list.includes(:last_build) if includes? 'repository.last_build'.freeze + list = list.includes(:default_branch) + list end end end diff --git a/lib/travis/api/v3/routes.rb b/lib/travis/api/v3/routes.rb index 18ea5ddc..66c13dde 100644 --- a/lib/travis/api/v3/routes.rb +++ b/lib/travis/api/v3/routes.rb @@ -11,6 +11,7 @@ module Travis::API::V3 resource :owner do route '/owner/({owner.login}|{user.login}|{organization.login})' get :find + get :repositories, '/repos' end resource :repository do diff --git a/lib/travis/api/v3/services/owner/repositories.rb b/lib/travis/api/v3/services/owner/repositories.rb new file mode 100644 index 00000000..77579ffd --- /dev/null +++ b/lib/travis/api/v3/services/owner/repositories.rb @@ -0,0 +1,10 @@ +module Travis::API::V3 + class Services::Owner::Repositories < Service + result_type :repositories + + def run! + unfiltered = query(:repositories).for_owner(find) + access_control.visible_repositories(unfiltered) + end + end +end From 99ca87b7c4b4dd77700f8d774f17f11efd1ef3ed Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Tue, 28 Apr 2015 12:41:29 +0200 Subject: [PATCH 2/6] API v3: make polymorphic has_many work --- lib/travis/api/v3/extensions/belongs_to.rb | 5 +++++ spec/v3/extensions/belongs_to_spec.rb | 1 + 2 files changed, 6 insertions(+) diff --git a/lib/travis/api/v3/extensions/belongs_to.rb b/lib/travis/api/v3/extensions/belongs_to.rb index 28aaa918..74bae01f 100644 --- a/lib/travis/api/v3/extensions/belongs_to.rb +++ b/lib/travis/api/v3/extensions/belongs_to.rb @@ -27,6 +27,11 @@ module Travis::API::V3 polymorfic_foreign_types << (options[:foreign_type] || "#{field}_type") if options[:polymorphic] super end + + def name + return super unless caller_locations.first.base_label == 'add_constraints'.freeze + @constraint_name ||= super.sub("#{parent}::", ''.freeze) + end end def self.included(base) diff --git a/spec/v3/extensions/belongs_to_spec.rb b/spec/v3/extensions/belongs_to_spec.rb index dd031ab5..b7a28042 100644 --- a/spec/v3/extensions/belongs_to_spec.rb +++ b/spec/v3/extensions/belongs_to_spec.rb @@ -13,5 +13,6 @@ describe Travis::API::V3::Extensions::BelongsTo do example { expect(repo.owner).to be_a(Travis::API::V3::Models::User) } example { expect(::Repository.find(repo.id).owner).to be_a(::User) } + example { expect(user.repositories).to include(repo) } end end \ No newline at end of file From 44f2be4afb79fd8ff8b049801382faddf07ab742 Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Tue, 28 Apr 2015 12:41:51 +0200 Subject: [PATCH 3/6] fix repo visibility filter --- lib/travis/api/v3/access_control/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/travis/api/v3/access_control/user.rb b/lib/travis/api/v3/access_control/user.rb index ff8eff44..f1979dfa 100644 --- a/lib/travis/api/v3/access_control/user.rb +++ b/lib/travis/api/v3/access_control/user.rb @@ -20,7 +20,7 @@ module Travis::API::V3 end def visible_repositories(list) - list.where('repositories.private = false OR repositories.id IN ?'.freeze, permissions.map(&:repository_id)) + list.where('repositories.private = false OR repositories.id IN (?)'.freeze, permissions.map(&:repository_id)) end protected From bbba06a85fb059fc6deb40f96127d17d27014222 Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Tue, 28 Apr 2015 12:43:54 +0200 Subject: [PATCH 4/6] API v3: fix owner query --- lib/travis/api/v3/services/owner/repositories.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/travis/api/v3/services/owner/repositories.rb b/lib/travis/api/v3/services/owner/repositories.rb index 77579ffd..1a025163 100644 --- a/lib/travis/api/v3/services/owner/repositories.rb +++ b/lib/travis/api/v3/services/owner/repositories.rb @@ -3,7 +3,7 @@ module Travis::API::V3 result_type :repositories def run! - unfiltered = query(:repositories).for_owner(find) + unfiltered = query(:repositories).for_owner(find(:owner)) access_control.visible_repositories(unfiltered) end end From bac3b38153617c42115d6ddb081bed1b115a6b34 Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Tue, 28 Apr 2015 12:44:20 +0200 Subject: [PATCH 5/6] API v3: specs for /owner/:login/repos endpoint --- spec/v3/services/owner/repositories_spec.rb | 69 +++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 spec/v3/services/owner/repositories_spec.rb diff --git a/spec/v3/services/owner/repositories_spec.rb b/spec/v3/services/owner/repositories_spec.rb new file mode 100644 index 00000000..8f686ebb --- /dev/null +++ b/spec/v3/services/owner/repositories_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::Owner::Repositories do + let(:repo) { Repository.by_slug('svenfuchs/minimal').first } + + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:headers) {{ 'HTTP_AUTHORIZATION' => "token #{token}" }} + before { Permission.create(repository: repo, user: repo.owner, pull: true) } + before { repo.update_attribute(:private, true) } + after { repo.update_attribute(:private, false) } + + describe "private repository, private API, authenticated as user with access" do + before { get("/v3/owner/svenfuchs/repos", {}, headers) } + example { expect(last_response).to be_ok } + example { expect(JSON.load(body)).to be == { + "@type" => "repositories", + "@href" => "/v3/owner/svenfuchs/repos", + "repositories" => [{ + "@type" => "repository", + "@href" => "/v3/repo/#{repo.id}", + "id" => repo.id, + "name" => "minimal", + "slug" => "svenfuchs/minimal", + "description" => nil, + "github_language" => nil, + "active" => true, + "private" => true, + "owner" => { + "@type" => "user", + "id" => repo.owner_id, + "login" => "svenfuchs" }, + "last_build" => { + "@type" => "build", + "@href" => "/v3/build/#{repo.last_build_id}", + "id" => repo.last_build_id, + "number" => "2", + "state" => "passed", + "duration" => nil, + "started_at" => "2010-11-12T12:30:00Z", + "finished_at" => "2010-11-12T12:30:20Z"}, + "default_branch" => { + "@type" => "branch", + "@href" => "/v3/repo/#{repo.id}/branch/master", + "name" => "master", + "last_build" => { + "@type" => "build", + "@href" => "/v3/build/#{repo.last_build.id}", + "id" => repo.last_build.id, + "number" => "3", + "state" => "configured", + "duration" => nil, + "started_at" => "2010-11-12T13:00:00Z", + "finished_at" => nil}}}] + }} + end + + describe "filter: private=false" do + before { get("/v3/repos", {"repository.private" => "false"}, headers) } + example { expect(last_response) .to be_ok } + example { expect(JSON.load(body)['repositories']) .to be == [] } + example { expect(JSON.load(body)['@href']) .to be == "/v3/repos?repository.private=false" } + end + + describe "filter: active=false" do + before { get("/v3/repos", {"repository.active" => "false"}, headers) } + example { expect(last_response) .to be_ok } + example { expect(JSON.load(body)['repositories']) .to be == [] } + end +end \ No newline at end of file From 595163619d3cc9c74baf8ab9e9cc425a5647dfe7 Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Tue, 28 Apr 2015 14:58:21 +0200 Subject: [PATCH 6/6] API v3: allow eager loading owner repos --- lib/travis/api/v3/renderer.rb | 11 ++-- lib/travis/api/v3/renderer/model_renderer.rb | 21 +++++--- lib/travis/api/v3/renderer/owner.rb | 16 +++++- lib/travis/api/v3/result.rb | 8 +-- lib/travis/api/v3/router.rb | 2 +- lib/travis/api/v3/service.rb | 8 ++- spec/v3/result_spec.rb | 10 ++-- spec/v3/services/owner/find_spec.rb | 54 +++++++++++++++++++- 8 files changed, 102 insertions(+), 28 deletions(-) diff --git a/lib/travis/api/v3/renderer.rb b/lib/travis/api/v3/renderer.rb index a2c2b1c5..c1518399 100644 --- a/lib/travis/api/v3/renderer.rb +++ b/lib/travis/api/v3/renderer.rb @@ -42,11 +42,12 @@ module Travis::API::V3 def render_value(value, **options) case value - when Hash then value.map { |k, v| [k, render_value(v)] }.to_h - when Array then value.map { |v | render_value(v) } - when *PRIMITIVE then value - when Time then value.strftime('%Y-%m-%dT%H:%M:%SZ') - when Model then render_model(value, **options) + when Hash then value.map { |k, v| [k, render_value(v)] }.to_h + when Array then value.map { |v | render_value(v) } + when *PRIMITIVE then value + when Time then value.strftime('%Y-%m-%dT%H:%M:%SZ') + when Model then render_model(value, **options) + when ActiveRecord::Relation then render_value(value.to_a, **options) else raise ArgumentError, 'cannot render %p (%p)' % [value.class, value] end end diff --git a/lib/travis/api/v3/renderer/model_renderer.rb b/lib/travis/api/v3/renderer/model_renderer.rb index 07944fec..2f65e201 100644 --- a/lib/travis/api/v3/renderer/model_renderer.rb +++ b/lib/travis/api/v3/renderer/model_renderer.rb @@ -34,15 +34,16 @@ module Travis::API::V3 new(model, **options).render(representation) end - attr_reader :model, :options, :script_name, :include, :included + attr_reader :model, :options, :script_name, :include, :included, :access_control attr_writer :href - def initialize(model, script_name: nil, include: [], included: [], **options) - @model = model - @options = options - @script_name = script_name - @include = include - @included = included + def initialize(model, script_name: nil, include: [], included: [], access_control: nil, **options) + @model = model + @options = options + @script_name = script_name + @include = include + @included = included + @access_control = access_control end def href @@ -69,7 +70,11 @@ module Travis::API::V3 nested_included = included + [model] modes = {} - excepted_type = result[:@type].to_s if include.any? + if include.any? + excepted_type = result[:@type].to_s + fields = fields.dup + end + include.each do |qualified_field| raise WrongParams, 'illegal format for include parameter'.freeze unless /\A(?\w+)\.(?\w+)\Z$/ =~ qualified_field next if prefix != excepted_type diff --git a/lib/travis/api/v3/renderer/owner.rb b/lib/travis/api/v3/renderer/owner.rb index 1d694b12..ead5edc0 100644 --- a/lib/travis/api/v3/renderer/owner.rb +++ b/lib/travis/api/v3/renderer/owner.rb @@ -5,7 +5,19 @@ module Travis::API::V3 class Renderer::Owner < Renderer::ModelRenderer include Renderer::AvatarURL - representation(:minimal, :id, :login) - representation(:standard, :id, :login, :name, :github_id, :avatar_url) + representation(:minimal, :id, :login) + representation(:standard, :id, :login, :name, :github_id, :avatar_url) + representation(:additional, :repositories) + + def initialize(*) + super + + owner_includes = include.select { |i| i.start_with?('owner.'.freeze) } + owner_includes.each { |i| include << i.sub('owner.'.freeze, "#{self.class.type}.") } + end + + def repositories + access_control.visible_repositories(@model.repositories) + end end end diff --git a/lib/travis/api/v3/result.rb b/lib/travis/api/v3/result.rb index cb7a8efb..1cf5733b 100644 --- a/lib/travis/api/v3/result.rb +++ b/lib/travis/api/v3/result.rb @@ -1,9 +1,9 @@ module Travis::API::V3 class Result - attr_accessor :type, :resource, :status, :href + attr_accessor :access_control, :type, :resource, :status, :href - def initialize(type, resource = [], status: 200) - @type, @resource, @status = type, resource, status + def initialize(access_control, type, resource = [], status: 200) + @access_control, @type, @resource, @status = access_control, type, resource, status end def respond_to_missing?(method, *) @@ -19,7 +19,7 @@ module Travis::API::V3 href = self.href href = V3.location(env) if href.nil? and env['REQUEST_METHOD'.freeze] == 'GET'.freeze include = params['include'.freeze].to_s.split(?,.freeze) - Renderer[type].render(resource, href: href, script_name: env['SCRIPT_NAME'.freeze], include: include) + Renderer[type].render(resource, href: href, script_name: env['SCRIPT_NAME'.freeze], include: include, access_control: access_control) end def method_missing(method, *args) diff --git a/lib/travis/api/v3/router.rb b/lib/travis/api/v3/router.rb index 956d3db9..74247095 100644 --- a/lib/travis/api/v3/router.rb +++ b/lib/travis/api/v3/router.rb @@ -21,7 +21,7 @@ module Travis::API::V3 result = service.run render(result, env_params, env) rescue Error => error - result = Result.new(:error, error) + result = Result.new(access_control, :error, error) headers = error.status == 404 ? CASCADE : {} V3.response(result.render(env_params, env), headers, status: error.status) end diff --git a/lib/travis/api/v3/service.rb b/lib/travis/api/v3/service.rb index 020a6aaf..c8c90249 100644 --- a/lib/travis/api/v3/service.rb +++ b/lib/travis/api/v3/service.rb @@ -61,9 +61,13 @@ module Travis::API::V3 self.class.result_type end + def result(*args) + Result.new(access_control, *args) + end + def run not_found unless result = run! - result = Result.new(result_type, result) unless result.is_a? Result + result = result(result_type, result) unless result.is_a? Result result end @@ -75,7 +79,7 @@ module Travis::API::V3 def accepted(**payload) payload[:resource_type] ||= result_type - Result.new(:accepted, payload, status: 202) + result(:accepted, payload, status: 202) end def not_implemented diff --git a/spec/v3/result_spec.rb b/spec/v3/result_spec.rb index b66945b3..3dd75885 100644 --- a/spec/v3/result_spec.rb +++ b/spec/v3/result_spec.rb @@ -1,11 +1,13 @@ require 'spec_helper' describe Travis::API::V3::Result do - subject(:result) { described_class.new(:example) } + let(:access_control) { Object.new } + subject(:result) { described_class.new(access_control, :example) } - example { expect(result.type) .to be == :example } - example { expect(result.resource) .to be == [] } - example { expect(result.example) .to be == [] } + example { expect(result.type) .to be == :example } + example { expect(result.resource) .to be == [] } + example { expect(result.example) .to be == [] } + example { expect(result.access_control) .to be == access_control } example do result << 42 diff --git a/spec/v3/services/owner/find_spec.rb b/spec/v3/services/owner/find_spec.rb index 6717047f..df026231 100644 --- a/spec/v3/services/owner/find_spec.rb +++ b/spec/v3/services/owner/find_spec.rb @@ -7,7 +7,7 @@ describe Travis::API::V3::Services::Owner::Find do after { org.delete } describe 'existing org, public api' do - before { get("/v3/owner/example-org") } + before { get("/v3/owner/example-org") } example { expect(last_response).to be_ok } example { expect(JSON.load(body)).to be == { "@type" => "organization", @@ -20,8 +20,58 @@ describe Travis::API::V3::Services::Owner::Find do }} end + describe 'eager loading repositories via organization.repositories' do + let(:repo) { Repository.new(name: 'example-repo', owner_name: 'example-org', owner_id: org.id, owner_type: 'Organization')} + + before { repo.save! } + after { repo.destroy } + + before { get("/v3/owner/example-org?include=organization.repositories,user.repositories") } + example { expect(last_response).to be_ok } + example { expect(JSON.load(body)).to be == { + "@type" => "organization", + "@href" => "/v3/org/#{org.id}", + "id" => org.id, + "login" => "example-org", + "name" => nil, + "github_id" => nil, + "avatar_url" => nil, + "repositories" => [{ + "@type" => "repository", + "@href" => "/repo/#{repo.id}", + "id" => repo.id, + "slug" => "example-org/example-repo" + }] + }} + end + + describe 'eager loading repositories via owner.repositories' do + let(:repo) { Repository.new(name: 'example-repo', owner_name: 'example-org', owner_id: org.id, owner_type: 'Organization')} + + before { repo.save! } + after { repo.destroy } + + before { get("/v3/owner/example-org?include=owner.repositories") } + example { expect(last_response).to be_ok } + example { expect(JSON.load(body)).to be == { + "@type" => "organization", + "@href" => "/v3/org/#{org.id}", + "id" => org.id, + "login" => "example-org", + "name" => nil, + "github_id" => nil, + "avatar_url" => nil, + "repositories" => [{ + "@type" => "repository", + "@href" => "/repo/#{repo.id}", + "id" => repo.id, + "slug" => "example-org/example-repo" + }] + }} + end + describe 'it is not case sensitive' do - before { get("/v3/owner/example-ORG") } + before { get("/v3/owner/example-ORG") } example { expect(last_response).to be_ok } example { expect(JSON.load(body)).to be == { "@type" => "organization",