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

Allow @odata.count to be written after enumeration. #2594

Open
uffelauesen opened this issue Jan 13, 2023 · 4 comments · May be fixed by #2595
Open

Allow @odata.count to be written after enumeration. #2594

uffelauesen opened this issue Jan 13, 2023 · 4 comments · May be fixed by #2595

Comments

@uffelauesen
Copy link
Contributor

In ODataJsonLightWriter.cs @odata.count annotations are written before enumeration of the resource set in StartResourceSetAsync or StartDeltaResourceSetAsync. This forces us to get the count calculated before we fetch data. In the implementation I have, this will require issuing a "select count() from..." query on my DB before the actual data query is issued "select a, b from...". In cases where all items in a resource set is returned in the payload we could avoid the "select count() from..." query as we already know the total number items.

@odata.nextLink annotation already behaves this way. It is either written early (before enumeration), if it set early in the resource set or after enumeration if it set later.

As far as I can read in the spec on odata.org, there is no mention of whether or not @odata.count should come early in the payload, so...

For performance reasons, please allow @odata.count annotations to be written in EndResourceSetAsync if not already written in StartResourceSetAsync or StartDeltaResourceSetAsync, like it is already possible with @odata.nextLink.

Assemblies affected

OData .Net lib 7.x

uffelauesen added a commit to uffelauesen/odata.net that referenced this issue Jan 13, 2023
@uffelauesen uffelauesen linked a pull request Jan 13, 2023 that will close this issue
2 tasks
@mikepizzo
Copy link
Member

Hi @uffelauesen --

Thanks for the investigation and for the contribution!

Note that, according to protocol, the count should be written before the values:

"The count name/value pair represents the number of entities in the collection. If present and the streaming=true content-type parameter is set, it MUST come before the value name/value pair."
The reason for this is because we wanted the client to be able to have this information before reading the response (i.e. in order to calculate memory or choose a deserialization pattern). For a single paged result, there is not much value adding the count at the end as the client could just count how many resources were received.

We allow the count to be approximate in order to account for the fact that the service may not accurately know the count until the end.

If the response does not follow ordering conventions (streaming != true), then it is valid to write the count at the end.

So, minimally, we need to update the PR to no write the count at the end if streaming=true. The question would be, do we throw an exception if streaming=true and the count is not present until the end, or do we just silently ignore the count if it wasn't specified until the end. It should be an error if the user requested the count and we don't provide it, but we can silently ignore if the count was just being provided as additional metadata.

@uffelauesen
Copy link
Contributor Author

Hi @mikepizzo

I see, thanks for the description.

It is actually when streaming=true, that I would have liked to be able to write the $count after the values.

The implementation I have uses server driven paging, so clients will not be able to use the $count to detect how much data will be sent in the values collection as the server potentially cuts the results and delivers the nextLink.

I have been using the $count=true feature to let the clients know the total amount of entities in a collection even if it has been truncated/paged. This information can be used by clients to show to the total count in a list together with a "next" button.

The performance penalty is not so much of an issue for the root collection, but for expanded collections (like TripPin: /People?$expand=Friends($select=Trips;$expand=Trips($count=true))), it could become problematic performance-wise for large collections an large server driven page sizes.

Maybe you could consider something like this to be added to a coming revision of the V4 protocol.

For now I will either de-support $count=true or supply the count value as a custom resource set annotation if possible.

Thanks for the feedback.
Uffe

@uffelauesen
Copy link
Contributor Author

Hi @mikepizzo

I will think about your suggestions some more and see if we can get something aligned.

I spent the day trying to get the count written after values in a custom annotation. It is possible, but clients would need to add the annotations writing in Prefer header.

I also noticed that missing counts are silently ignored even if requested by client as it is now (before my PR changes). If this current behaviour is valid - then I can’t see why it should not be allowed to write it at the end. I have to think about this a bit more.

Uffe

@uffelauesen
Copy link
Contributor Author

It is not allowed to write custom annotations on expanded resource sets neither before or after the values.
It is specifically for expanded resource sets that the ability to write odata.count after the values can have huge performance benefits.
Please consider allowing writing odata.count after values for both root resource set and axpanded resource sets as described in this issue report.
ODataReader is already capable of reading odata.count after values both on root and on expanded resource sets.

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 a pull request may close this issue.

2 participants