Potential Object Design Issue

[mascon]

There is a issue with the design of the user object and if not carefully used data is lost.

This is caused by the way the information is collected into object. Box only returns the following elements by default.

id name login created_at modified_at language space_amount space_used max_upload_size status job_title phone address avatar_url

These elements are only returned on request. role is_sync_enabled can_see_managed_users tracking_codes is_exempt_from_device_limits is_exempt_from_login_verification

That leaves us with a bug when using the following code.

    Dim ManagedUsers = BoxClient.GetUsers("someuser@yourdomain.com")
    Dim ManagedUser = BoxClient.GetUser(ManagedUsers.Entries(0).Id)

    'Update Box information
    With ManagedUser
        .Phone = "ThePhoneNumber"
    End With

    BoxClient.UpdateUser(ManagedUser)

This updates all the elements in the ManagedUser object however not all the values were collected and hence default values are posted back to box overwriting the values for this user.

[jhoerr]

Well that's a bummer. Not totally sure the best way to handle that -- need to give it some thought.

[mascon]

Its worst that I first thought

Even if you explicty define the fields to update like so

BoxClient.UpdateUser(ManagedUser, New Field() {Field.Phone, Field.Status})

It still clears the values!

Running an emergency script to re-enable 6000 sync users!

This leaves the user update as next to unusable right now.

[jhoerr]

Crikey -- sorry! The fields argument only applies to the data that gets returned from Box, not the data that gets sent.

Working on a fix now.

[jhoerr]

Fixed in e87f125d450eaa93c93ee08d8fcd06e0df4ccbeb. Added a new flag to the UpdateUser method by which you must explicitly specify to include the enterprise user fields in the update request.

[mascon]

Not sure if I understand this change but looking at the code only as I cannot reset the VS right now as it is running the fix. Will take another hour or so to complete from the hotel here in NL.

Do I now need to add a fields list when I add the adminfields = true if I want to include any of the below fields?

name role language is_sync_enabled job_title phone address space_amount tracking_codes can_see_managed_users status is_exempt_from_device_limits is_exempt_from_login_verification

[jhoerr]

The fields parameter has nothing whatsoever to do with the data that gets sent to Box. It strictly controls what data Box returns. If no fields argument is supplied, then Box will return 'everything' for that User, File, &c. The User is an odd duck because there are certain enterprise fields that Box only returns by request. (The reason for this is not obvious to me.) So if you want to get any of the enterprise fields in the response, you must create a fields list that contains the 'enterprise' Field enum(s) in addition to which (if any) of the 'non-enterprise' Field enum(s) that you'd like to get back. Does this make sense? It's a little hard to explain; this particular API design decision seems to complicate things a bit.

// Update the user without setting any enterprise fields
client.UpdateUser(user);

// Update the user and set the enterprise fields, 
//     and just get the non-enterprise properties in the response.
client.UpdateUser(user, true);

// Update the user, set the enterprise fields, and get 
//     the enterprise properties in the response
client.UpdateUser(user, true, new[]{ Field.Name, Field.Role... });

[mascon]

Then do you think that the GetUser should also be updated with the same design?

BoxClient.GetUser(ManagedUsers.Entries(0).Id, true) Gets the enterprise user settings

BoxClient.GetUser(ManagedUsers.Entries(0).Id, false) Returns the standard users settings.

BoxClient.GetUser(ManagedUsers.Entries(0).Id, true, Fields) Returns the fields requested

-----Original Message----- From: John Hoerr notifications@github.com To: jhoerr/box-csharp-sdk-v2 box-csharp-sdk-v2@noreply.github.com Cc: mascon imom@andimgone.com Date: Mon, 07 Jan 2013 14:24:12 -0800 Subject: Re: [box-csharp-sdk-v2] Potential Object Design Issue (#11)

The fields parameter has nothing whatsoever to do with the data that gets sent to Box. It strictly controls what data Box returns. If no fields argument is supplied, then Box will return 'everything' for that User, File, &c. The User is an odd duck because there are certain enterprise fields that Box only returns by request. (The reason for this is not obvious to me.) So if you want to get any of the enterprise fields in the response, you must create a fields list that contains the 'enterprise' Field enum(s) in addition to which (if any) of the 'non-enterprise' Field enum(s) that you'd like to get back. Does this make sense? It's a little hard to explain; this particular API design decision seems to complicate things a bit.

// Update the user without setting any enterprise fields client.UpdateUser(user);

// Update the user and set the enterprise fields, // and just get the non-enterprise properties in the response. client.UpdateUser(user, true);

// Update the user, set the enterprise fields, and get // the enterprise properties in the response client.UpdateUser(user, true, new[]{ Field.Name, Field.Role... }); — Reply to this email directly or view it on GitHub.

[jhoerr]

It seems like Box's intention was to make it non-trivial to get the enterprise fields. I'd like to learn more about why they made that choice before I make it trivial. Let me see what I can find out.

[jhoerr]

Ok, I think I've resolved this issue in fd1c75163693081a83deebc959dcb9959b67f055. Take a look at the change notes and let me know if you have any questions or concerns. I was able to make things the enterprise user methods a good bit simpler, which I usually take as a good sign.

[mascon]

So now to use just the user fields I would

Dim ManagedUser as User

Correct?

[jhoerr]

I'm not sure I understand the question -- can you describe the scenario?

[prexer]

John,

Just to answer your earlier question about why we made the enterprise fields on the /user object not part of the default response. In general, we expect the /user object to be called by an application to get the info about the currently logged in user to show it to that user, in which case the enterprise settings will largely be irrelevant. We also figured that the enterprise admins would be the more sophisticated developers, and wouldn't have problems with adding ?fields to the requests.

[jhoerr]

Hi Peter, thanks for that insight. I think it makes sense. I sent a similar question to api@box.com and got a similarly good explanation from Sean. I regret making statements above that now seem critical of y'all's design choices here. I was frustrated at the time but not yet being able to see (what I think is) the whole problem.

The challenge for me is that I declaratively prescribe which /user properties are serialized in a POST/PUT. These declarations aren't conditional or stateful; I can't say 'only serialize this property if it was previously fetched.' This has led to the unintentional overwriting of data when those enterprise /user fields that aren't returned by default are PUT to Box with their default (wrong) values. (This has caused some real headaches for at least one very gracious early adopter.) I could write custom serializer(s) to handle this, but those wheels are best left unreinvented.

The way I've worked around this is simply to take fields out of the equation for the enterprise user methods. All enterprise requests fetch the full compliment of fields and thus don't have to be concerned about which fields to PUT for an update. This, to me, seems to be the best way for the SDK to guarantee data integrity without resorting to custom serializers and/or burdening the SDK user with a lot of decisions.

Which is all to say: I think you guys made the right choice, and I think I've figured out the best -- or at least a reasonable -- path given the set of constraints that I've chosen to work within.

[prexer]

If I'm hearing you correctly, it sounds like it might be a bug.

You got back some fields you then did a PUT without those fields, and we reset you back to defaults for those fields?

-Peter

On Thu, Jan 10, 2013 at 9:41 AM, John Hoerr notifications@github.comwrote:

nd this is simply to take fields out of the equation for the enterprise user methods. All enterprise requests fetch the full compliment of fields and thus don't have to be concerned about which fields to PUT for an update. This, to me, seems to be the best way for the

[jhoerr]

It's not a Box bug; I wasn't communicating very well. :) I'll take two swings at explaining it again.

Swing One: A subset of fields was returned. An object was initialized with that subset, and some properties -- not having a corresponding field returned -- were initialized with their default values ('false' in the case of bools, etc.) When the object is serialized for the PUT, all of the properties -- those originally returned by Box and those not -- are included in the serialization. The serializer has no way of knowing that some of those properties are, essentially, invalid. So as a result of the PUT, the fields that were not originally returned are clobbered by the .Net defaults for a given field's type.

Swing Two: Assume there was some Box class, /thing, that had two Boolean fields, A and B. On the server, both are true. During a GET request, only field 'A' is requested.

1. GET: Box sends down: {"A": true}.
2. .Net initializes: new Thing(){ A = true }, which leaves B to it's default value of false.
3. PUT: .Net serializes the object and sends back: {"A" : true, "B" : false}, thus clobbering the real value.

I think it's fair to say each actor in that scenario is, as far as it knows, doing the 'right' thing. That's what made it a tricky issue to resolve (for me anyway.)

[prexer]

Gotcha. So C# wants to serialize every property of the object, which kinda makes sense... though it could break things...

-Peter

On Thu, Jan 10, 2013 at 5:50 PM, John Hoerr notifications@github.comwrote:

It's not a Box bug; I wasn't communicating very well. :) I'll take two swings at explaining it again.

1.

A subset of fields was returned. An object was initialized with that subset, and some properties -- not having a corresponding field returned -- were initialized with their default values ('false' in the case of bools, etc.) When the object is serialized for the PUT, all of the properties -- those originally returned by Box and those not -- are included in the serialization. The serializer has no way of knowing that some of those properties are, essentially, invalid. So as a result of the PUT, the fields that were not originally returned are clobbered by the .Net defaults for a given field's type. 2.

Assume there was some Box class, /thing, that had two Boolean fields, Aand B. On the server, both are true. During a GET request, only field 'A' is requested.

GET: Box sends down: {"A": true}. .Net initializes: new Thing(){ A = true }, which leaves B to it's default value of false. PUT: .Net serializes the object and sends back: {"A" : true, "B" : false}, thus clobbering the real value.

I think it's fair to say each actor in that scenario is, as far as it knows, doing the 'right' thing. That's what made it a tricky issue to resolve (for me anyway.)

— Reply to this email directly or view it on GitHubhttps://github.com/jhoerr/box-csharp-sdk-v2/issues/11#issuecomment-12128473.

[jhoerr]

You can tell it to ignore certain properties, but it's totally declarative, and thus always-include or always-ignore. I think that ultimately the onus is upon the developer to structure the class hierarchy so that they're only serializing the subset of data that they really want to send.