Add paging links to the API #15148

jntullo · 2017-05-18T14:04:44Z

This adds paging links / page count in the following format if both limit and offset are specified in the GET request:

GET http://www.example.com/api/vms?offset=2&limit=2&sort_by=name&expand=resources
{
  "name"=>"vms",
 "count"=>7,
 "subcount"=>2,
 "pages"=>4,
 "resources"=>
  [
   {"href"=>"http://www.example.com/api/vms/10000000007254",
    "id"=>10000000007254,
    "vendor"=>"vmware",
    "name"=>"cc",
...
    "tenant_id"=>10000000007758
   },
   {"href"=>"http://www.example.com/api/vms/10000000007257",
    "id"=>10000000007257,
    "vendor"=>"vmware",
    "name"=>"dd",
...
    "tenant_id"=>10000000007758
   }
],
 "actions"=>[{"name"=>"query", "method"=>"post", "href"=>"http://www.example.com/api/vms"}],
 "links"=>
  {
   "self"=>"http://www.example.com/api/vms?offset=2&limit=2&sort_by=name&expand=resources",
   "next"=>"http://www.example.com/api/vms?offset=4&limit=2&sort_by=name&expand=resources",
   "previous"=>"http://www.example.com/api/vms?offset=0&limit=2&sort_by=name&expand=resources",
   "first"=>"http://www.example.com/api/vms?offset=0&limit=2&sort_by=name&expand=resources",
   "last"=>"http://www.example.com/api/vms?offset=6&limit=2&sort_by=name&expand=resources"
   }
}

If there is no previous (ie you are on first), there will be no previous link. This holds true for the other cases such as being on the last, etc.

Things still working on:

Additional tests
Ensuring that filtering works / makes sense: for filtering, it is necessary to have a subcount. currently, the counts we have are total count (which is the count on the collection) and subcount (number that are being returned), which is not sufficient. This PR does include a hotfix to get a subquery count, but am looking at a better way

@miq-bot add_label api, enhancement, wip
@miq-bot assign @abellotti
cc: @imtayadeway

jntullo · 2017-05-23T20:33:30Z

@miq-bot remove_label wip

imtayadeway

@jntullo this is looking great! I just have some questions above. Feel free to 👖 if you need to discuss. Thanks!

imtayadeway · 2017-05-23T21:14:22Z

lib/api/link_builder.rb

@@ -0,0 +1,81 @@
+module Api
+  class LinkBuilder


Did you consider LinksBuilder for this? I think this could better convey intent, since it returns a thing comprised of links

imtayadeway · 2017-05-23T21:15:03Z

lib/api/link_builder.rb

@@ -0,0 +1,81 @@
+module Api
+  class LinkBuilder
+    PAGING_LINKS = [


I think you want s/paging/pagination/ here

imtayadeway · 2017-05-23T21:18:46Z

lib/api/link_builder.rb

+    end
+
+    def links
+      PAGING_LINKS.each_with_object({}) do |link, object|


What do you think about expressing this as a literal (over metaprogramming) here? Something like:

{ :self => self, :next => next, # etc => etc }.compact

imtayadeway · 2017-05-23T21:20:16Z

lib/api/link_builder.rb

+      format_href(prev_offset)
+    end
+
+    def self


Overriding self here may have unintended consequences....perhaps self_link?

imtayadeway · 2017-05-23T21:22:39Z

lib/api/link_builder.rb

+    end
+
+    def paging_count
+      @paging_count ||= subquery_count.nil? ? count : subquery_count


WDYT about the (slightly) shorter:

@paging_count ||= subquery_count || count

imtayadeway · 2017-05-23T21:42:48Z

app/controllers/api/base_controller/renderer.rb

        Jbuilder.new do |json|
          json.ignore_nil!
-          [:name, :count, :subcount].each do |opt_name|


Does this disrupt the other caller of this method (expand_subcollection) which still sends a :count option?

imtayadeway · 2017-05-23T21:47:15Z

lib/api/link_builder.rb

+      :last
+    ].freeze
+
+    attr_reader :offset, :limit, :href, :count, :subcount, :subquery_count


I couldn't find a consumer of the first reader - offset, outside the class. WDYT about giving this and similar attributes private readers instead? I think this would help make the responsibilities of this class clearer

imtayadeway · 2017-05-23T21:49:56Z

lib/api/link_builder.rb

+      @limit = params["limit"].to_i if params["limit"]
+      @href = href
+      return unless counts
+      @count = counts[:count]


Not necessary (but food for thought), the number of parameters needed by this object suggest that it might be helpful to Introduce Parameter Object

imtayadeway · 2017-05-23T21:57:53Z

lib/api/link_builder.rb

+    def previous
+      return if offset.zero?
+      prev_offset = offset - limit
+      return if prev_offset < 0


This seems to suggest that if I ask for page with limit 10, offset 5, I won't see a previous link showing the first five results. I think I'd expect to see a link (expressing limit 10, offset -5) though I'm not sure if that makes sense. @AllenBW any thoughts?

hm yeah, this is a tough one. I guess I was just thinking first would cover this case, but perhaps they don't want that. Thoughts @AllenBW ?

UGGGHHHH sorry for the delayed response to this, it totally didn't hit my 📻

showing all links no matter what offset would be besssssssst. the example case being... lets say you wanta make a list of the different pages (say 1 to 10) and when you click a number you see that cut of results, if we start with 5 we would wanta also be able to checkout 4, 3, etc etc

when ya say, @jntullo first would cover this case, watcha mean?

again so many apologies for my late reply here :( here have an I'm sorry 🌮

@AllenBW with the first link, the offset would be 0, at which point you'd be able to see 0..5. But I could see the case for wanting to continue with that same offset

OOOOOO so collection of ten limit of 2 offset of 6, would you have links for offset of 4, 2 and 0? @jntullo

No, you would get previous which would be offset of 4, you would get first, which would be offset of 0, you'd get next which would be offset of 8

imtayadeway · 2017-05-23T22:03:33Z

lib/api/link_builder.rb

+    attr_reader :offset, :limit, :href, :count, :subcount, :subquery_count
+
+    def initialize(params, href, counts)
+      @offset = params["offset"].to_i if params["offset"]


Do you need to pass in the params if you have what you need in the href arg, and that url needs to be parsed/rebuilt as part of this logic anyway?

@imtayadeway not a bad suggestion, the only reason why I am not entirely sold on it is the rebuilding part is a bit simpler than the parsing part, and if we already have those parameters, I would prefer to just use those rather than re-parsing it. If that makes sense? Open to discuss though.

chessbyte · 2017-06-02T03:58:37Z

@jntullo bump

miq-bot · 2017-06-14T14:18:27Z

This pull request is not mergeable. Please rebase and repush.

abellotti · 2017-07-06T15:59:06Z

app/controllers/api/base_controller/renderer.rb


-        Rbac.filtered(res, options)
+      def subquery_search(res, options)


not clear offhand what subquery_search does (as called from 179 if miq_expression.present), might need a better name

abellotti · 2017-07-06T16:00:26Z

lib/api/links_builder.rb

+      @limit = params["limit"].to_i if params["limit"]
+      @href = href
+      return unless counts
+      @counts = counts


maybe replace above 2 lines with

@counts = counts if counts

abellotti · 2017-07-06T16:03:49Z

lib/api/links_builder.rb

+    def last_href
+      return if (offset + limit) >= paging_count
+      last_offset = paging_count - (paging_count % limit)
+      format_href(last_offset)


while I can see us skipping previous and next (i.e you're in the first page already or you're in the last so no need to paginate further), I think we should always return the self, first and last href's.

abellotti · 2017-07-06T16:05:29Z

This is really nice @jntullo
Minor fix needed,

for limit=12 offset=10, previous was limit=12 offset=-2, I expected that to be limit=12 offset=0 (i.e. same as first)

have the link builder take in the count parameters

update specs to be more descriptive;

abellotti · 2017-07-24T21:23:53Z

Thanks @jntullo for updating this PR. LGTM!! 😍

/cc @imtayadeway does this look good to you ? you have the X in the reviewers section.

rubocop fix

miq-bot · 2017-07-25T11:33:25Z

Checked commits jntullo/manageiq@f10ea5a~...c4d59f6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 4 offenses detected

app/controllers/api/base_controller/renderer.rb

❗ - Line 34, Col 16 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 38, Col 20 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 42, Col 16 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 60, Col 22 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

jntullo · 2017-07-25T11:36:23Z

Looks like the rubocop.yml was updated - there are a lot of those errors throughout renderer, will fix all of them separately.

miq-bot added api enhancement wip labels May 18, 2017

miq-bot assigned abellotti May 18, 2017

jntullo changed the title ~~[WIP] Add paging links to the API~~ Add paging links to the API May 23, 2017

miq-bot removed the wip label May 23, 2017

imtayadeway suggested changes May 23, 2017

View reviewed changes

miq-bot added the unmergeable label Jun 14, 2017

miq-bot removed the unmergeable label Jun 22, 2017

abellotti reviewed Jul 6, 2017

View reviewed changes

Jillian Tullo added 8 commits July 24, 2017 15:46

abstract paging into its own class

f10ea5a

add paging links to requests with both offset and limit specified

8f00ec8

addressing additional cases

71473dc

return page counts

e6339e5

account for filtering

9d4353c

create links builder

afeeb04

have the link builder take in the count parameters

add query_counts.rb;

ffe76e1

update specs to be more descriptive;

do not return negative offsets

1d1dea4

always return self, first, last

c4d59f6

rubocop fix

imtayadeway approved these changes Jul 26, 2017

View reviewed changes

abellotti added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 27, 2017

abellotti merged commit ad21f82 into ManageIQ:master Jul 27, 2017

jntullo deleted the enhancement/paging branch November 28, 2017 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add paging links to the API #15148

Add paging links to the API #15148

jntullo commented May 18, 2017

jntullo commented May 23, 2017

imtayadeway left a comment

imtayadeway May 23, 2017

imtayadeway May 23, 2017

imtayadeway May 23, 2017

imtayadeway May 23, 2017

imtayadeway May 23, 2017

imtayadeway May 23, 2017

imtayadeway May 23, 2017

imtayadeway May 23, 2017

imtayadeway May 23, 2017

jntullo Jun 8, 2017

AllenBW Jun 12, 2017

jntullo Jun 13, 2017

AllenBW Jun 13, 2017

jntullo Jun 13, 2017

imtayadeway May 23, 2017

jntullo Jun 8, 2017

chessbyte commented Jun 2, 2017

miq-bot commented Jun 14, 2017

abellotti Jul 6, 2017

abellotti Jul 6, 2017

abellotti Jul 6, 2017

abellotti commented Jul 6, 2017

abellotti commented Jul 24, 2017

miq-bot commented Jul 25, 2017

jntullo commented Jul 25, 2017


		Rbac.filtered(res, options)
		def subquery_search(res, options)

Add paging links to the API #15148

Add paging links to the API #15148

Conversation

jntullo commented May 18, 2017

jntullo commented May 23, 2017

imtayadeway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chessbyte commented Jun 2, 2017

miq-bot commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Jul 6, 2017

abellotti commented Jul 24, 2017

miq-bot commented Jul 25, 2017

jntullo commented Jul 25, 2017