jhoerr / box-csharp-sdk-v2

A C# client for the Box API (v2).
http://developers.box.com/docs/
11 stars 15 forks source link

Various Issues with GetFolder and GetItems #27

Closed mascon closed 11 years ago

mascon commented 11 years ago

When I call the following code.

    Dim FolderList = BoxClient.GetFolder("96842004", New FolderField() {FolderField.Parent, FolderField.ItemCollection, FolderField.Size, FolderField.OwnedBy, FolderField.Name, FolderField.PathCollection}).Folders

    For Each Folder In FolderList

        FolderPath = PathBuilder(Folder)

        folderInformation.Rows.Add(Folder.Parent.Id, Folder.Id, Folder.Name, FolderPath, "", Folder.Size)

    Next

I have the following issues.

1) I can find no way of returning more that 100 items. The API uses Limit and Offset for this but I can not find this replicated within the SDK.

2) When calling the method I have to include any fields I want except for ID and Name. However looking at the resulting JSON returned the the SDK other elements are populated by default

3) Even though I include the FolderField.ItemCollection nothing is returned.

jhoerr commented 11 years ago
  1. The Limit/Offset is a new feature and we'll need to add support for it. But per the documentation it should only apply to the BoxManager.GetItems() method -- are you certain you're seeing the ItemCollection limited to 100 items on a call to BoxManager.GetFolder()?

2/3: I think the current ItemCollection implementation may be creating a bad expectation here. By definition the API returns a strict set of data for each item in the ItemCollection: type, id, name, etag, and sha1 (for files). By making the ItemCollection a list of `Folder` objects it gives the impression that more data can be provided. I need to tighten this up; it was a bad design decision.

That said, the `fields` parameter for BoxManger.GetFolder() applies only to the properties returned for the folder itself, *not* for the properties in the ItemCollection.

If you want to get information about the `Size` and `PathCollection` for each item within a folder, you'll need to use the BoxManager.GetItems() method. For this method the `fields` parameter limits the properties returned for each the contained items (as opposed to those of the folder itself.)

Edit: I was wrong about the claims I made in 2/3. See update below.

mascon commented 11 years ago

1) I'm getting nothing back at all in the items collection. I managed to fake the offset and limit calls by hard coding them in the request helper.

I agree with the fields. However your not returning the default returned fields only I'D and name. So to get the defaults I need to request them to be returned.

What I am looking for is the number of files in the folder tree. The size is the collated value and the web interface shows the number of files.

If this is available as an api call I could not find it hence I looked at the items collection.

jhoerr commented 11 years ago

Can you provide a postman trace of the failing call?

John Hoerr (mobile) 503.360.6581

On Feb 16, 2013, at 5:43 PM, mascon notifications@github.com wrote:

1) I'm getting nothing back at all in the items collection. I managed to fake the offset and limit calls by hard coding them in the request helper.

I agree with the fields. However your not returning the default returned fields only I'D and name. So to get the defaults I need to request them to be returned.

What I am looking for is the number of files in the folder tree. The size is the collated value and the web interface shows the number of files.

If this is available as an api call I could not find it hence I looked at the items collection.

— Reply to this email directly or view it on GitHub.

mascon commented 11 years ago

Well I'm just on the iPad and heading to bed. Tomorrow I will deliver everything to you.

But let's get on the right page.

I'm calling getfolder(ID).folders and having in the return object the items collection is nothing. I did not check the Json but will tomorrow for you. The question is what should I find in there.

I think you need to know there is only folders in the parent folder I request. Since these are returned I wonder if the items collection would be empty anyhow.

jhoerr commented 11 years ago

Update to previous claims. I was wrong about a few things above and want to correct them.

Folder and ItemCollection properties

The fields passed into GetFolder() will affect which properties are fetched for the folder and the objects in its ItemCollection. Some examples to illustrate this point:

0. Properties that are set regardless of field specification

As best as I can determine the following are always true:

1. GetFolder with no fields specified

var folder = Client.GetFolder(testFolder.Id);
var items = folder.ItemCollection;
GET /2.0/folders/673353682 HTTP/1.1

In this case, folder would have all of its properties set and items would only have the default mini file/folder properties set: type, id, name, etag, and sha1 (for files).

2. GetFolder with fields specified

var folder = Client.GetFolder(testFolder.Id, new[] { FolderField.PathCollection });
var items = folder.ItemCollection;
GET /2.0/folders/673418936?fields=path_collection HTTP/1.1

In this case, the folder and all items would be set with the type, id, etag, and pathCollection properties.

3. GetItems

This works the same way as GetFolder.

ItemCollection Limit/Offset

Per the documentation this feature only applies to the GetItems method. However, it appears that Folder.ItemCollection that is populated following a call to GetFolder is also limit/offset-bound. I don't know whether this is the intended behavior and I'll file an issue with the Box folks.

mascon commented 11 years ago

I look forward to the response from Box so we can get this sorted in the SDK.... You love the way I used "we" in that sentence :-)

jhoerr commented 11 years ago

synchronous GetFolder() calls now supports pagination as of 6db88885dcd84642026095904ea54ea36fbb0655.

Pagination will effectively be disabled by default. If you don't specify a limit or offset, it is assumed that you want to fetch the entire item collection. The SDK will execute as many requests as necessary in order to get everything.

I'll try get support for GetItems() done tomorrow.

mascon commented 11 years ago

Fantastic news,

I was thinking about the fact that we return everything by default and wonder if that is wise.

If I look at how the .Net framework handles AD Calls by default it returns a max 1000 records. You can send a page size and offset if you wish the trawl through the results or you can set the pagesize and tell the offset to be unlimited. I know that a little confusing but was their design.

The framework then uses the page size internally to return all the results.

Now the question is why would the design engineers choose that approach. Because they don't know if there is 10000 records or a 1000000.

Its the same for us. I just wander about the extreamely long call and the potential massive array of objects this could result in and could we be designing a problem into the SDK.

Just my thoughts for your review.

jhoerr commented 11 years ago

My uneducated-but-inspired opinion is that most folks won't ever have this issue. Causal users of Box simply don't keep that many items in a single folder.

I made this design choice in order that the SDK would continue to work as before without any additional development effort. Super massive file trees will need special consideration, and insofar as they are not the norm (if you accept my argument above) then I think this is reasonable.

jhoerr commented 11 years ago

Synchronous Folder.GetItems() calls now support pagination as of fecab7c010010b4299209eb0747283a390ed727d

jhoerr commented 11 years ago

Async GetFolder() and GetItems() calls support pagination as of 6cb4ca53041ba71be14b30ece4b1dabcd5d158c1. These work a little differently than their synchronous counterparts. Where they sync calls will get everything in the folder if the limit and offset are not specified, the async calls do not. This is intentional: async programming tends towards pagination, but I wanted to introduce this as a non-breaking change.