box / box-windows-sdk-v2

Windows SDK for v2 of the Box API. The SDK is built upon .NET Framework 4.5
https://developer.box.com
Apache License 2.0
186 stars 163 forks source link

GetEnterpriseUsersWithMarkerAsync throws NRE when evaluating Entries #948

Open watfordsuzy opened 6 months ago

watfordsuzy commented 6 months ago

Description of the Issue

If I include an externalAppUserId to a call to GetEnterpriseUsersWithMarkerAsync the resulting marker based collection has a null Entries field.

Steps to Reproduce

string? marker = null;
do
{
    var result = await client.UsersManager.GetEnterpriseUsersWithMarkerAsync(marker, externalAppUserId: "user_id");
    foreach (BoxUser user in result.Entries) // NullReferenceException (result.Entries)
    {
        // ...
    }

    marker = result.NextMarker;
}
while (!string.IsNullOrWhiteSpace(marker));

Expected Behavior

The list of entries is enumerated.

Error Message, Including Stack Trace

This is in a durable function app and the stack trace is a bit useless.

Screenshots

Versions Used

.Net SDK: 5.7.0 Windows: dotnet-isolated|8.0

watfordsuzy commented 6 months ago

Worse still, calling GetEnterpriseUsersAsync also throws an error when I include the external app ID.

watfordsuzy commented 6 months ago

I now think this is a regression with the main Box API, how do I get this escalated?

https://gist.github.com/watfordsuzy/c5f85f1099e259f64e5acb4b0881d18f

ref: https://developer.box.com/guides/api-calls/pagination/marker-based/

mwwoda commented 6 months ago

I just checked and this endpoint behaves very strangely with different payload. When useMarker is included along with external_app_user_id, instead of returning collection in entries field as usual it returns it in the users field, which is not even present in the spec. As a result SDK cannot deserialize it because of field name mismatch and entries field is null.

I could only reproduce this issue only when used marker so far (couldn't reproduce with GetEnterpriseUsersAsync only with GetEnterpriseUsersMarkerBasedAsync). At the first glance, response in your example from GetEnterpriseUsersAsync looks fine. Not sure why you got NRE here.

You could try contacting Box support at https://forum.box.com/top?period=quarterly. I'll contact the service owners on our side to get first-hand feedback.

mwwoda commented 6 months ago

You also mentioned regression? Could you elaborate on that? Was it working fine for you in the past?

watfordsuzy commented 6 months ago

Yes, all of this code was working for me previously. Seems only recently that the addition of external_app_user_id causes the weird response from the server.

EDIT: I've also raised an issue on the new support forums. https://support.box.com/hc/en-us/community/posts/29468435097747-Users-API-regression-with-usemarker-parameter

watfordsuzy commented 6 months ago

It looks like you were correct and I can successfully work around the issue using GetEnterpriseUsersAsync. Not 100% sure why I saw the NRE, might have been an old instance of the code was picked up by the function app host.

mwwoda commented 6 months ago

Service owners confirmed the issue and they looking into a fix. I don't have any timeframe for now, but I'll let you know in case of any updates. For now, my advice is to stick with GetEnterpriseUsersAsync function.

watfordsuzy commented 6 months ago

@mwwoda thank you, will do!

Is it even possible to have two box users with the same external_app_user_id?

congminh1254 commented 6 months ago

Hi @watfordsuzy

As I tested, so it's not possible to have two Box users with the same external_app_user_id.

watfordsuzy commented 6 months ago

@mwwoda @congminh1254 I received the following from Box Support on my case and I'm not sure how to manage this with the C# library:

Case: #3099758 Hi Christopher,

Thanks for reaching out. My name is Dan and I'm a Platform specialist here at Box. I understand then when using the .NET SDK, that when you pass a external_app_user_id parameter along with a usermarker=true parameter to a List Enterprise Users call that marker-based pagination is not being used.

When using marker-based pagination, a marker field will only be returned in the response when there are enough entries to warrant multiple pages of results. Where an external_app_user_id is specified only a single entry will ever be returned, and so a marker field will not be included in the results even when marker-based pagination is specified.

I hope this is helpful. Please let me know if you have any questions or if I may be of any further assistance.

I asked a follow-up question about the stability of the APIs when requesting Marker Based results and received the following:

Hi Christopher,

Inasmuch as it's not always possible to know whether enough entries will be returned to allow for multiple page of results, it will not always be definitively possible to know whether results will be returned in a marker-based format. In this connection, the best way forward will be to write the program in such a way as to be able to useful parse results in either format.

I can definitely see how it would be useful for the API to always return results in the same format when marker based pagination is specified, even in the event that only a single page of results is returned. You can flag the need for this change at pulse.box.com. Our engineers regularly review this platform to inform their future priorities and roadmaps.

stale[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not been updated in the last 30 days. It will be closed if no further activity occurs within the next 7 days. Feel free to reach out or mention Box SDK team member for further help and resources if they are needed.

watfordsuzy commented 5 months ago

@mwwoda @congminh1254 thoughts on this? I think the SDK should be modified such that this doesn't break unexpectedly. Either by:

  1. Not allowing the external app ID when you use marker based pagination
  2. "Fixing" the response from the API to match the contract of the marker parameter (very bizarre to me the API gets to ignore that field in certain situations).
mwwoda commented 4 months ago

@watfordsuzy I just pinged the team to check the state of the fix on the backend. Ideally, using externalAppId should not lead to this behaviour, and SDK fix should not be necessary. If it cannot be fixed on the service side for some reason, then we need to adjust the SDK.