BCF-API icon indicating copy to clipboard operation
BCF-API copied to clipboard

Add note about pagination

Open jasollien opened this issue 4 years ago • 10 comments

jasollien avatar Oct 19 '20 10:10 jasollien

@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?

ykulbak avatar Oct 19 '20 12:10 ykulbak

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)

Amoki avatar Oct 19 '20 13:10 Amoki

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.

GeorgDangl avatar Oct 19 '20 13:10 GeorgDangl

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

Amoki avatar Oct 19 '20 13:10 Amoki

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.

GeorgDangl avatar Feb 26 '24 09:02 GeorgDangl

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?

NKnusperer avatar Jun 19 '24 13:06 NKnusperer

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]

GeorgDangl avatar Jun 19 '24 20:06 GeorgDangl

@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.

NKnusperer avatar Jul 15 '24 09:07 NKnusperer

@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.

cosmo0 avatar Jul 18 '24 12:07 cosmo0

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

GeorgDangl avatar Aug 26 '24 09:08 GeorgDangl