Expose rubygem reverse depencencies #945

Merged
merged 5 commits into from Dec 28, 2016

Projects

None yet
@spk
Contributor
spk commented Apr 29, 2015 edited

rails_reverse_dependencies_search_jquery

  • rebase after #1498 merged
  • Fix rubocop on rubygem_helper class
  • Extract aside template
  • Improve reverse deps queries, (ref #1508)
@dwradcliffe
Member

Can you post a screenshot?

@spk
Contributor
spk commented Apr 29, 2015

@dwradcliffe yes it's on the description

@sferik
Member
sferik commented Apr 29, 2015

As the maintainer of many gems, this information is incredibly useful to me—knowing which gems depend on mine, so I can see how they’re being used—but I remember when we benchmarked this query it was rather slow. Since this information is not broadly useful (to 99% of gem users vs. 1% of gem maintainers), it may make more sense to put on a separate page, so the query doesn’t need to run every time someone looks at a gem page.

Also, have you tested that this will work on specific gem version pages? For example if gem a depends on gem b ~> 1.0, gem a should appear as a reverse dependency for gem b version 1.0.0 and 1.9.9, but not 0.9.9 or 2.0.0. There may already be unit tests for this case but I’d feel better if we had functional/controller tests covering it.

@spk
Contributor
spk commented Apr 30, 2015

I see what you mean @sferik, and I did not try with the production database.
I will work on a separate page version, with versions selection.
Thank you for the feedback.

@sferik
Member
sferik commented Apr 30, 2015

@spk 👍 Thank you for working on this feature. I’m really looking forward to the day when it’s merged and deployed.

@spk
Contributor
spk commented May 1, 2015

With latest production database, with rails the query is about 151.6ms and give 8073 reverse dependencies to rails gem!
So yes another page with version and pagination is required!

@simi
Contributor
simi commented May 1, 2015

Maybe it will be handy to sort gems by download count.

@spk
Contributor
spk commented May 2, 2015

I think I have a usable feature, with pagination, search filter and ordered by download (thanks @simi).
For versions I'm using the same method as in the API, but it don't filter with major/minor version @sferik.

@parndt
Contributor
parndt commented May 10, 2015

As a gem maintainer, this is a really helpful feature.. thank you!

@spk
Contributor
spk commented May 11, 2015

Thanks! Is it possible to have a review by the team and get it merge ?

@sferik
Member
sferik commented May 11, 2015

👍 Looks good to me. I’d like to see a 👍 from at least one other maintainer before merging.

/cc @qrush @evanphx @skottler @dwradcliffe

@qrush
Member
qrush commented May 11, 2015

A few comments-

  1. I feel this would been cleaner implemented as ReverseDependenciesController - anything that isn't a RESTful action tends to be a smell, eventually. That can wait for a refactoring PR if anyone is up for it.
  2. The HTML view is pretty barebones. Can we show the same partial as on searches#show, and possibly get that cached?
  3. We'll have to watch performance a little more carefully when this is deployed - at least paginating it helps.
@simi
Contributor
simi commented May 11, 2015

I'm going to review this today.

@spk
Contributor
spk commented Jul 2, 2015

Status since last review:

  • Performance issue fixed by 3e13be0
  • Nicer templates using existing and share aside sidebar
@oz
oz commented Jul 5, 2015

This would be a nice feature. I hope you guys find time to merge this. 👍

@qrush
Member
qrush commented Jul 7, 2015

Is there any way we can get this on a "feature flag" type system? So if we deploy it and need to turn it off fast w/o rollback that would be feasible. Any thoughts @dwradcliffe @arthurnn ?

@dwradcliffe
Member

@qrush good idea 👍

@arthurnn
Member
arthurnn commented Jul 8, 2015

"feature flag" , should be easy to make it work. 👍 from me.. the only thing I hate about those is that people tend to leave features behind, and the code start getting 2+ code paths.. As long as we remember that. I am fine

@bf4
Contributor
bf4 commented Jul 17, 2015

Great job! These pages would definitely need to be throttled so someone couldn't use it to DDoS the site ;) That is, they should have a default/fallback state of 'unavailable' at this time when the data is not available, for whatever reason.

Also, any consideration to do this in JavaScript via the API, perhaps like gemloupe did? (bookmarklet below)

javascript:(function(){var script=document.createElement('SCRIPT');script.src='https://www.gemlou.pe/assets/remote_loupe_consumer.js';document.body.appendChild(script);})()

I'd be happy to do the JS as an Ember Component

@qrush
Member
qrush commented Jul 17, 2015

What if we pre-generated all of the reverse dependencies for every gem and cached them? How much space/memory would that require? We could even have a new Delayed::Job to produce this on gem push.

Sent from my iPhone

On Jul 16, 2015, at 9:10 PM, Benjamin Fleischer notifications@github.com wrote:

Great job! These pages would definitely need to be throttled so someone couldn't use it to DDoS the site ;) That is, they should have a default/fallback state of 'unavailable' at this time when the data is not available, for whatever reason.

Also, any consideration to do this in JavaScript via the API, perhaps like gemloupe did? (bookmarklet below)

javascript:(function(){var script=document.createElement('SCRIPT');script.src='https://www.gemlou.pe/assets/remote_loupe_consumer.js';document.body.appendChild(script);})()
I'd be happy to do the JS as an Ember Component


Reply to this email directly or view it on GitHub.

@bf4
Contributor
bf4 commented Jul 17, 2015

Except we'd need to update it on every gem import, though that may be a cheaper query as it's a direct dep

@spk
Contributor
spk commented Jul 27, 2015

I see that there is on the Gemfile dalli gem but it does not look used ?

@spk spk referenced this pull request Jul 27, 2015
Closed

Remove unused gem dalli #1033

@sferik
Member
sferik commented Jul 27, 2015

@spk Yes, we stopped using dalli in 7e4c702 but the dependency was never removed from the Gemfile.

@bf4
Contributor
bf4 commented Jul 28, 2015

@sferik @qrush @dwradcliffe @arthurnn @evanphx @skottler Any consensus re: displaying calculating and displaying reverse dependencies on web pages?

  • where the data should be stored
    • db
    • cache
  • when the data should be calculated
  • page load
  • eventual consistency is ok with everyone
    • gem import
    • background job
  • how the data should be queried
    • as part of the page load
    • on demand: by javascript over the api
  • how it should be displayed
    • on its own page
    • on each version's page
    • paginated
  • how it should be rolled out
    • feature flag
  • benchmark threshold for performance cost of implementation?
  • some subset of the above in this pr, and the rest in future prs?
@spk
Contributor
spk commented Sep 2, 2015

Rebased and caching enabled for reverse dependencies pages

@dwradcliffe
Member

Just realized we never responded to the list of questions. I'd like to try with generating the page on demand and cache in memcached. If we need to roll back or disable for performance reasons we can do that pretty easily. (This may be what you have now - I haven't looked closely yet.)

@spk
Contributor
spk commented Nov 1, 2015

Is there still an interest for this PR ? I still wait for feedback.

@oz
oz commented Nov 2, 2015

Well... yes. Hopefully, someone can have a look at this soon-ish. 😃

@arthurnn arthurnn self-assigned this Nov 4, 2015
@spk
Contributor
spk commented Nov 27, 2015

Ref #123

@kbrock kbrock commented on an outdated diff Mar 29, 2016
app/controllers/versions_controller.rb
def index
@versions = @rubygem.versions.by_position
end
def show
- @latest_version = Version.find_from_slug!(@rubygem.id, params[:id])
@versions = @rubygem.public_versions_with_extra_version(@latest_version)
@kbrock
kbrock Mar 29, 2016 Contributor

if you're going to lazy load latest_version probably want to remove the @ from this line

@kbrock
kbrock Mar 29, 2016 Contributor

take this back. the before_action will load this.

@kbrock kbrock and 1 other commented on an outdated diff Mar 29, 2016
app/helpers/rubygems_helper.rb
@@ -54,36 +55,42 @@ def unsubscribe_link(rubygem)
else
'display:none'
end
- link_to t('.links.unsubscribe'), rubygem_subscription_path(rubygem),
+ link_to t('rubygems.show.links.unsubscribe'),
+ rubygem_subscription_path(rubygem),
@kbrock
kbrock Mar 29, 2016 Contributor

nice.

If this pr is not ready to merge, may make sense to just pull these bug fixes out of here.

@spk
spk Apr 10, 2016 Contributor

I've tried to extract performance issue and bug while working on this PR but this is my own bug from creating a _aside.html.erb partial in 3504859
I will rebase and squash this fix to avoid misunderstanding

@homu
Collaborator
homu commented May 25, 2016

☔️ The latest upstream changes (presumably #1291) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Collaborator
homu commented May 29, 2016

☔️ The latest upstream changes (presumably #1294) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Collaborator
homu commented Jun 5, 2016

☔️ The latest upstream changes (presumably #1307) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Collaborator
homu commented Jun 20, 2016

☔️ The latest upstream changes (presumably #1333) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Collaborator
homu commented Aug 27, 2016

☔️ The latest upstream changes (presumably #1370) made this pull request unmergeable. Please resolve the merge conflicts.

@indirect
Member

@spk any interest in rebasing this for a merge soon? if not, I think @sonalkr132 would be willing to work on it. 👍

@spk
Contributor
spk commented Aug 27, 2016

@indirect yes I will finish that this week end; look like there is an issue now with the legacy_search, I will inspect that.

@sonalkr132
Member

@spk can you please open an issue about what you encountered?

@spk
Contributor
spk commented Aug 27, 2016

@sonalkr132 yep done, cf #1395

+ def latest_version
+ @latest_version ||= @rubygem.versions.most_recent
+ end
+
@dwradcliffe
dwradcliffe Sep 26, 2016 Member

I'm not sure ApplicationController is the right place for this.

@spk
spk Sep 27, 2016 Contributor

This is because its used on ReverseDependenciesController and RubygemsController with before_action like find_rubygem, what do you recommend instead?

@brodock
brodock Oct 4, 2016

@spk perhaps extract into a concern?

@spk
spk Oct 4, 2016 Contributor

Thanks @brodock applied in 8dce272

@sonalkr132 sonalkr132 added the backlog label Oct 27, 2016
@homu
Collaborator
homu commented Nov 6, 2016

☔️ The latest upstream changes (presumably #1433) made this pull request unmergeable. Please resolve the merge conflicts.

app/helpers/rubygems_helper.rb
def documentation_link(version, linkset)
return unless linkset.nil? || linkset.docs.blank?
link_to_page :docs, version.documentation_path
end
def badge_link(rubygem)
badge_url = "https://badge.fury.io/rb/#{rubygem.name}/install"
- link_to t(".links.badge"), badge_url, class: "gem__link t-list__item", id: :badge
+ link_to t("rubygems.show.links.badge"), badge_url,
+ class: "gem__link t-list__item", id: :badge
@sonalkr132
sonalkr132 Nov 19, 2016 Member

@spk You can change this back to one line to fix rubocop error. We can refactor RubygemsHelper in a different PR.

+ should "show reverse dependencies" do
+ get :index, rubygem_id: @rubygem_one.to_param
+ assert page.has_content?(@rubygem_two.name)
+ assert !page.has_content?(@rubygem_three.name)
@sonalkr132
sonalkr132 Nov 19, 2016 Member

Please use refute here and in ones down below.

@spk spk added a commit to spk/rubygems.org that referenced this pull request Nov 19, 2016
@spk spk Fix rubocop error d54e2bd
@spk spk added a commit to spk/rubygems.org that referenced this pull request Nov 19, 2016
@spk spk Use refute instead of assert not 53b651c
app/views/rubygems/show.html.erb
- <%= atom_link(@rubygem) %>
- <%= report_abuse_link(@rubygem) %>
- </div>
- </div>
@sonalkr132
sonalkr132 Nov 19, 2016 Member

Can you please send this change of moving aside bar to partial in a different PR? Also, instead of following:

-    link_to(t(".links.#{id}"), url, rel: 'nofollow', class: ['gem__link', 't-list__item'], id: id) if url.present?
+    link_to(t("rubygems.show.links.#{id}"), url, rel: 'nofollow', class: ['gem__link', 't-list__item'], id: id) if url.present?

move all this out of show to aside.

@spk
spk Nov 19, 2016 Contributor

@sonalkr132 already tried to do that on #1141
but did not have interest, I will do that again and apply your propositions

@sonalkr132
sonalkr132 Nov 19, 2016 Member

oh! 😅 Sorry @spk. I think we can use it.

Also, sorry again for delay with this PR. I look forward to getting this merged once I am sure about impact it might have on our servers. Checking out your branch locally 😉

Thank you so much for writing this https://www.spkdev.net/2015/05/23/reverse-dependencies-with-rubygems.html ❤️

@spk
spk Nov 19, 2016 Contributor

Great @sonalkr132 ! Thanks !

@spk spk added a commit to spk/rubygems.org that referenced this pull request Nov 19, 2016
@spk spk Extract aside template and related locales fd38cf2
@sonalkr132 sonalkr132 assigned sonalkr132 and unassigned arthurnn Nov 19, 2016
@homu
Collaborator
homu commented Nov 19, 2016

☔️ The latest upstream changes (presumably #1506) made this pull request unmergeable. Please resolve the merge conflicts.

@sonalkr132
Member

@spk I have bad news 😓

It has n+1 for gem_download. Also, Versions.reverse_dependencies(name) is too effin slow 😭

  def self.reverse_dependencies(name)
    joins(dependencies: :rubygem)
      .indexed
      .where(rubygems: { name: name })
  end

I think first finding the gem and then using id instead of name would be way faster. Do you think you can benchmark two options and open an issue?

@spk
Contributor
spk commented Nov 21, 2016

@sonalkr132 not that bad !
For the record those methods are already used on the API
I can work on that maybe later this week

@sonalkr132
Member

I can work on that maybe later this week

Sure 👍

@kbrock
Contributor
kbrock commented Nov 22, 2016

not sure if this needs an index. And does this want to be case insensitive? (use lower)

+ before_action :set_page, only: [:index]
+
+ def index
+ @latest_version = @rubygem.versions.most_recent
@sonalkr132
sonalkr132 Dec 17, 2016 Member

Isn't @latest_version already set with before_action :latest_version?

@spk
spk Dec 18, 2016 Contributor

Yes its a mistake; fixed in 61a5a90

+<header class="gems__header push">
+ <%= form_tag rubygem_reverse_dependencies_path(@rubygem),
+ :id => "rdeps-search", :class => "header__search-wrap", :method => :get do %>
+ <%= search_field_tag :rdeps_query, params[:rdeps_query], :placeholder => "Search Reverse Deps Gems&hellip;".html_safe, :class => "header__search" %>
@sonalkr132
sonalkr132 Dec 17, 2016 Member

Please use I18n for placeholder.

@spk
spk Dec 18, 2016 Contributor

Fixed in 5ef01ec

+ :id => "rdeps-search", :class => "header__search-wrap", :method => :get do %>
+ <%= search_field_tag :rdeps_query, params[:rdeps_query], :placeholder => "Search Reverse Deps Gems&hellip;".html_safe, :class => "header__search" %>
+ <%= label_tag :rdeps_query do %>
+ <span class="t-hidden">Search gems</span>
@sonalkr132
sonalkr132 Dec 17, 2016 Member

Please use I18n.

@spk
spk Dec 18, 2016 Contributor

Fixed in 5ef01ec

+ before_action :set_page, only: [:index]
+
+ def index
+ @reverse_dependencies = @rubygem.reverse_dependencies.by_downloads
@sonalkr132
sonalkr132 Dec 21, 2016 Member

Please use @rubygem.reverse_dependencies.by_downloads.includes(:latest_version, :gem_download) instead.

@spk
spk Dec 21, 2016 Contributor

see 012c1e2

@@ -0,0 +1,12 @@
+<header class="gems__header push">
+ <%= form_tag rubygem_reverse_dependencies_path(@rubygem),
+ :id => "rdeps-search", :class => "header__search-wrap", :method => :get do %>
@sonalkr132
sonalkr132 Dec 21, 2016 Member

Please use ruby 1.9 hash syntax.

@spk
spk Dec 21, 2016 Contributor

see fa0f34b

spk added some commits Nov 20, 2016
@spk spk Expose rubygem reverse depencencies
* /gems/:rubygem_id/reverse_dependencies
* search on reverse dependencies
fa8ab20
@spk spk Remove duplicate @latest_version load 2cd4e19
@spk spk Use I18n for search reverse deps input 2a25ff4
@spk spk Use Ruby 1.9 hash syntax for _reverse_dependencies.html fa0f34b
@spk spk Include latest_version and gem_download for reverse
Thanks-to: @sonalkr132
012c1e2
@indirect
Member

@homu r+

@homu
Collaborator
homu commented Dec 28, 2016

📌 Commit 012c1e2 has been approved by indirect

@homu homu added a commit that referenced this pull request Dec 28, 2016
@homu homu Auto merge of #945 - spk:expose-reverse-dependencies, r=indirect
Expose rubygem reverse depencencies

![rails_reverse_dependencies_search_jquery](https://cloud.githubusercontent.com/assets/98590/7667144/605ee3a4-fbfe-11e4-94ae-6d71442aa5a5.png)

- [x] rebase after #1498 merged
- [x] Fix rubocop on rubygem_helper class
- [x] Extract aside template
- [x] Improve reverse deps queries, (ref #1508)
9769a74
@homu
Collaborator
homu commented Dec 28, 2016

⌛️ Testing commit 012c1e2 with merge 9769a74...

@homu
Collaborator
homu commented Dec 28, 2016

☀️ Test successful - status

@homu homu merged commit 012c1e2 into rubygems:master Dec 28, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@spk spk deleted the spk:expose-reverse-dependencies branch Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment