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

Rust re-write dropped some metadata keys of paginated response #11

Open
ayalash opened this issue Nov 19, 2019 · 10 comments
Open

Rust re-write dropped some metadata keys of paginated response #11

ayalash opened this issue Nov 19, 2019 · 10 comments

Comments

@ayalash
Copy link
Contributor

ayalash commented Nov 19, 2019

From the (old) Python implementation of mailboxer

@blueprint.route("/mailboxes")
@paginated_view(renderer=_render_mailbox)
@sorted_view(allowed_fields=["last_activity", "id"])
def list_mailboxes():
    return Mailbox.query  # pylint: disable=no-member
@blueprint.route("/mailboxes/<address>/emails")
@paginated_view
def list_all_mailbox_emails(address):
    _check_mailbox_exists(address)
    return Email.query.join(Mailbox).filter(Mailbox.address == address).order_by(Email.timestamp)  # pylint: disable=no-member

While paginated_view is from weber_utils package adding the following fields to the metadata of these views: page, page_size, total_num_objects, total_num_pages

https://github.com/vmalloc/weber-utils/blob/master/weber_utils/pagination.py#L9-L32

The rust implementation is lacking these fields. For example:

[2019-11-19 17:18:39.304720] DEBUG: mailboxer.utils: [mailboxer.lab.gdc.il.infinidat.com] GET /v2/mailboxes, data: None
[2019-11-19 17:18:39.321138] DEBUG: mailboxer.utils: [mailboxer.lab.gdc.il.infinidat.com] 200 {"metadata":{"page":1},"result":[{"address":"some_address","id":1817,"last_activity":{"nanos_since_epoch":139825000, "secs_since_epoch":1574175632}}]}
@ayalash
Copy link
Contributor Author

ayalash commented Dec 11, 2019

I tried fixing this issue by adding the following lines:

$ git diff src/results.rs
diff --git a/src/results.rs b/src/results.rs
index 79838c3..092c015 100644
--- a/src/results.rs
+++ b/src/results.rs
@@ -2,6 +2,8 @@ use crate::pagination::Pagination;
 use actix_web::{Error, HttpRequest, HttpResponse, Responder, Result};
 use serde::Serialize;
 use serde_json::json;
+use math::round;
+
 
 pub struct Success;
 
@@ -30,6 +32,9 @@ impl<T: Serialize> Responder for APIResult<T> {
                 "result": results,
                 "metadata": {
                     "page": pagination.get_page(),
+                    "page_size": pagination.get_page_size(),
+                    "total_num_pages": round::ceil((results.len() as i64 / pagination.get_page_size()) as f64, 0),
+                    "total_num_objects": results.len(),
                 }
             }),
             APIResult::SingleResult(result) => json!({

The problem is that in pagination variable is after "cutting" the results according to the page.
@vmalloc I'll be glad for your instructions

@vmalloc
Copy link
Member

vmalloc commented Dec 15, 2019

@ayalash how critical are the "total_num_objects" and "total_num_pages"? They are expensive to produce database-wise...

@ayalash
Copy link
Contributor Author

ayalash commented Dec 15, 2019

@vmalloc the "total_num_objects" itself is not critial, but as long as our metadata contains only "page" and "page_size" we cannot know if there are more results or not.

@vmalloc
Copy link
Member

vmalloc commented Dec 15, 2019

When you get less than page_size results per page, you reached the end...

@ayalash
Copy link
Contributor Author

ayalash commented Dec 15, 2019

And when the results is multiple of "page_size", you should continue (extra API) until the page will be empty?
It is not that elegant, but we can live with it...

@vmalloc
Copy link
Member

vmalloc commented Dec 15, 2019

Yes. That's how most "streaming" APIs work. I can also provide a "has next page" attribute to save that extra page fetch. However providing the total number of objects is expensive because it often requires a full database scan...

@ayalash
Copy link
Contributor Author

ayalash commented Dec 15, 2019

Backslash API has "has_more" metadata. I think that if we'll have a similar flag it is enough :)

@vmalloc
Copy link
Member

vmalloc commented Dec 15, 2019

Ok. I'll add it.

@ayalash
Copy link
Contributor Author

ayalash commented Dec 15, 2019

Thanks

@vmalloc
Copy link
Member

vmalloc commented Dec 15, 2019

@ayalash develop has the fix already - let me know if it's working ok so that I can publish a release.

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

No branches or pull requests

2 participants