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

Add note about pagination #230

Open
wants to merge 1 commit into
base: release_3_0
Choose a base branch
from
Open

Conversation

jasollien
Copy link
Contributor

No description provided.

@ykulbak
Copy link
Collaborator

ykulbak commented Oct 19, 2020

@jasollien is it common to specify odata.totalCount and odata.nextLink in a header (and not the json payload)? Is this a workaround to avoid a backward incompatible change to the payload?

@Amoki
Copy link

Amoki commented Oct 19, 2020

I've seen many API using headers to add some context to the query (not with odata though).
It may be less convenient sometimes but changing the body response is a hard task.

A dot in a header name seems to be forbidden (https://tools.ietf.org/html/rfc7230#section-3.2.6)

@GeorgDangl
Copy link
Member

GeorgDangl commented Oct 19, 2020

I'm also not a huge fan of using headers to transport data myself, since it makes client implementations harder. So maybe we should keep the headers optional?

Also I think we should prepend the headers with a x- to indicate that they're custom headers.

@Amoki
Copy link

Amoki commented Oct 19, 2020

x- for custom header has been deprecated (https://tools.ietf.org/html/rfc6648 and https://stackoverflow.com/a/3561399). I think a prefix to avoid any name conflict could be great

@ykulbak ykulbak added this to the V2.3 milestone Nov 16, 2020
@ykulbak ykulbak modified the milestones: V2.3, V4.0 Nov 25, 2020
@ykulbak ykulbak changed the base branch from release_2_2 to release_3_0 November 25, 2020 08:43
Copy link
Contributor

@pasi-paasiala pasi-paasiala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a bit more information in the commit message. Is this related to some issue? Content of the change seems fine.

@GeorgDangl
Copy link
Member

This is a pretty old PR, but still an important one😀

We've just discussed this in the call, and it looks like there's more work to do. The basic facts right now are:

  • When we return paginated results / list results, we're directly returning an array like [...] of items from the BCF API
  • OData (and most other implementations, too) seem to assume that when you return a list, you wrap it in a response object like { "values": [...] }
  • Wrapping gives you flexibility, e.g. you can add additional properties like a totalCount, currentPage or similar to the response
  • The current version of OData does not seem to use the headers as suggested in this PR, but only relies on custom properties, e.g. @odata.totalCount in the wrapping model

-> We need to spend a bit of time on that issue. In the discussion, we felt that we probably have to modify all the responses for list results to return wrapping elements, but this is a bit of work.

@NKnusperer
Copy link

Any updates on this topic?
Also, while wondering how to implement pagination, I've noticed that this is currently the only issue blocking the V4 release according to the milestone. Is this true?

@GeorgDangl
Copy link
Member

Hey @NKnusperer, we haven't had a chance to look at this yet in the past few weeks. At the moment, we're focusing on updating the swagger.yaml specification, so that one will be the source of truth for the API.

We don't really have a schedule yet for when we're getting to this, so can't give you a date there☹ We're meeting every 14 days on zoom, and work through current issues. If you'd like to join, you're always welcome! Just send me an email, or directly to [email protected]

@NKnusperer
Copy link

@GeorgDangl @Amoki after some research, I think the best solution is to use headers as already suggested.
This way the body does not need to be changed and there is a clear separation between content and metadata.
In fact, there is already a specification for this, Web Linking (RFC 5988), which is used by the GitHub REST API, for example.

@cosmo0
Copy link

cosmo0 commented Jul 18, 2024

@GeorgDangl @Amoki after some research, I think the best solution is to use headers as already suggested. This way the body does not need to be changed and there is a clear separation between content and metadata. In fact, there is already a specification for this, Web Linking (RFC 5988), which is used by the GitHub REST API, for example.

The "link" headers seem to be a hassle to implement, but I agree: adding headers is the only way to not break every single existing implementation of the standard. Adding an object wrapper would need a rewrite of both server and client, and don't get me started on servers implementing both v3 and v4 of the API. Adding headers like BCF-Results-Total and BCF-Results-MaxItems would allow, in combination with the $skip and $top OData parameters, standard and easy pagination.

However, I don't agree with the current pull request: paginations are there for a performance reason, and sending all of the data by default is a terrible idea. I don't know what you guys work with, but we have many clients with several hundred thousands, sometimes millions of projects, and must join with several tables to check for various things. I would rather not send one and a half million items through the API.

@GeorgDangl
Copy link
Member

@jasollien we need to update this to reflect the new format of the pagination responses.

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

Successfully merging this pull request may close these issues.

7 participants