Add remove method to backing store

[mkomko]

This remove method would be very useful to us.

Our use case is the following: We are mapping a user of our application to an Entra ID User object. This includes the manager field. But the manager cannot be updated using a PATCH call to the user, but has to be updated with a separate call. So we would still like to do the mapping into the manager field, but remove it before sending the actual PATCH request.

[mkomko]

@microsoft-github-policy-service agree

[baywet]

Hi @mkomko Thanks for starting this pull request, I think we need to discuss the scenario further here.

on adding a remove method to the backing store.

Adding a method to an interface is a source breaking change. The proper way of doing that would be to add an additional derived interface for the remove method and mark the base one as deprecated. Then, once we major rev, we could clean this up.

Another challenge here is that this would likely be a change we need to align across all languages, not just java, for consistency.

on this specific scenario

Since this is a Graph scenario, I'd like to explore alternative solutions before we commit to all that extra work here. Can you provide a snippet of the code you're implementing here please? Solutions would include:

• using select on the initial query to exclude the manager property
• mapping the desired properties to a new object, so only those are carried over

[mkomko]

Hi @baywet

Thank you very much for your comment!

Adding a method to an interface is a source breaking change.

You're correct, I didn't think about this, sorry! What would you say about adding it with a default implementation that throws an UnsupportedOperationException? I don't know if default implementations exist in all required languages though.

Another challenge here is that this would likely be a change we need to align across all languages, not just java, for consistency.

Also correct. Would that be something you could do on your side or would you have me do that? It would be tougher to do for me since I don't have dev environments for all languages. But if you'd look it over I could try.

Can you provide a snippet of the code you're implementing here please?

Sure. I'll just paste parts of the code here that demonstrate my problem (Graph SDK V5). There is much more code relying on this behavior in my application.

// -----------------------------------------------------------------------------
//  Load current user
// -----------------------------------------------------------------------------
User currentUser = graph.getUserById(office365Id)

// -----------------------------------------------------------------------------
//  Create new user using field mapping
// -----------------------------------------------------------------------------
User newUser = toUser(fieldMapping, currentUser.userPrincipalName)

// -----------------------------------------------------------------------------
//  Update user by patching changes
// -----------------------------------------------------------------------------
List<String> changedProperties = []
User userPatchObject = createUserPatchObject(currentUser, newUser, fieldMapping, changedProperties)

// -----------------------------------------------------------------------------
//  Manager cannot be set directly
// -----------------------------------------------------------------------------
DirectoryObject manager = null
boolean updateManager = false
if (changedProperties.contains("manager"))
{
    updateManager = true
    manager = userPatchObject.manager
    userPatchObject.manager = null
    userPatchObject.additionalDataManager().remove("manager")
}

// -----------------------------------------------------------------------------
//  Update user
// -----------------------------------------------------------------------------
graph.updateUserById(currentUser.id, userPatchObject)

// -----------------------------------------------------------------------------
//  Update user's manager
// -----------------------------------------------------------------------------
if (updateManager)
    graph.updateUserManager(currentUser.id, manager != null ? manager.id : null)

So basically the toUser method uses a FieldMapping to create a new user object with the data currently in my application with the goal to update all user fields that changed. Since the manager is part of the field mapping, the User#manager field changed.

The only other solutions I could come up with are:

• Create a wrapper object around the User, that passes everything to the User but the manager
• Create yet another new User object and somehow set all of the "changedProperties" except the manager (which is what you're suggesting I guess)

Those would be quite invasive changes (also because there is other similar code that also relies on the ability to remove something from being sent to the API).

Of course this is something I could work around, but I believe that this functionality should actually exist and could generally be of use.

What do you think? Thanks again!

[baywet]

Thanks for the additional context here. I don't want to dive too deep in your code, but I think the sequence/logic needs to be updated here: Why do you need to reset the manager property if you're already creating a new patch object? Wouldn't it be simpler to have the patch object creation NOT map the manager to begin with? The concerns mixing of "updating user's properties" and "updating user's manager" is probably what's making things extra difficult. Assuming you update the code to remove that mix of concerns, you shouldn't need a remove method anymore.

[mkomko]

@baywet Thank you for your quick answer.

Why do you need to reset the manager property if you're already creating a new patch object? Wouldn't it be simpler to have the patch object creation NOT map the manager to begin with?

I don't think it would be simpler, but it would solve the immediate problem I have with the SDK. But it would be against the separation of concerns. I do not want the mapping logic to have to know about the API's "shortcomings" of not being able to set the manager directly. And I would have to check if the manager field is even supposed to be mapped and thus updated separately.

So of course it's possible to work around that, but I would prefer the proposed solution. There is a BackingStore#clear method, so it would only seem logical to also have a remove method for a single item. But if it's too much trouble and you can't see the benefits I'd have to accept that.

The concerns mixing of "updating user's properties" and "updating user's manager" is probably what's making things extra difficult.

In my application, the manager is a field as any other. In the Graph API it's not.

Actually, one could argue, that since the manager field can never be set this way in the User object, it should never be added to the backing store in the first place! That would also solve the problem ;-). Maybe you can add some "non-updatabable" flag to the data from which the SDK is generated and ignore those fields in the backing store?

[baywet]

Thanks for the additional information here. Ah... Don't get me started with the design flaws of navigation properties in OData... (which is the modeling protocol for Microsoft Graph).

An excellent alternative which is a bit more advanced than what most people do would be to use odata bind to update everything at once.

userPatchObject.getAdditionalDataManager().put("manager@odata.bind": "https://graph.microsoft.com/v1.0/users/Manager-UID");
// also make sure the method creating the patch object is NOT setting the user's manager

Doing so would allow you to drop this statement all together

if (updateManager)
    graph.updateUserManager(currentUser.id, manager != null ? manager.id : null)

[mkomko]

That sounds very interesting, I will have to give it a try on Monday. Thank you very much!

How would I remove the manager using this syntax?

[baywet]

Try passing null. Alternatively you can call client.Users["userId"].Manager.Ref.DeleteAsync()

[mkomko]

Setting the manager directly using odata.bind works nicely, thank you very much for the tip!

I was not able to use it to remove the manager, though:

user.getAdditionalData().put("manager@odata.bind", null);

"Invalid value for manager@odata.bind property"

user.getAdditionalData().put("manager@odata.bind", "null");

Invalid URL format specified in @odata.bind for manager

user.getAdditionalData().put("manager@odata.bind", "https://graph.microsoft.com/v1.0/users/null");

Resource 'null' does not exist or one of its queried reference-property objects are not present.

user.getAdditionalData().put("manager", null);

Invalid Property manager

Any ideas? That would indeed solve our most immediate problem! Thanks in advance!

[baywet]

I am guessing because of the ambiguity of my previous response, you wrote null with double quotes as a string. Have you tried passing null the code symbol instead? From a syntax perspective it seems to be correct. https://learn.microsoft.com/en-us/power-apps/developer/data-platform/webapi/associate-disassociate-entities-using-web-api#disassociate-with-a-single-valued-navigation-property

[mkomko]

Thank you for your reply. In my comment above (maybe you missed parts of it?) you can see that I tried both and the error messages I received.

[baywet]

Sorry about that. Can you please try serializing an logging the payload with the KiotaJsonSerialization static helpers to see what the payload looks like please?

[mkomko]

You probably need the request payload? Because I can give you the response payload, but cannot log the request payload due to this bug: https://github.com/microsoftgraph/msgraph-sdk-java/issues/2037

Do you have any code examples for these KiotaJsonSerialization static helpers?

[mkomko]

I found another way:

This:

user.getAdditionalData().put("manager@odata.bind", null);

Results in:

{"manager@odata.bind":null,"@odata.context":"https://graph.microsoft.com/v1.0/$metadata#users(id,accountEnabled,assignedLicenses,assignedPlans,deletedDateTime,displayName,isResourceAccount,lastPasswordChangeDateTime,licenseAssignmentStates,mail,mailNickname,onPremisesDistinguishedName,onPremisesImmutableId,onPremisesLastSyncDateTime,onPremisesSecurityIdentifier,onPremisesSyncEnabled,onPremisesDomainName,onPremisesSamAccountName,onPremisesUserPrincipalName,showInAddressList,usageLocation,userPrincipalName,userType,businessPhones,city,companyName,country,department,employeeHireDate,employeeId,employeeOrgData,employeeType,faxNumber,givenName,jobTitle,mobilePhone,officeLocation,postalCode,preferredLanguage,state,streetAddress,surname,manager,manager())/$entity"}

[baywet]

Thanks for the additional information. Besides the OData context, the request body looks correct. I'm not sure why the service would be unhappy with it. You might want to remove the odata context and see if you still face the issue, just for sanity.

[mkomko]

The request now looks as follows: {"@odata.type":"#microsoft.graph.user","manager@odata.bind":null}

But the result is, sadly, the same: Invalid value for manager@odata.bind property

[baywet]

Thanks for the additional information. I suggest you open a support ticket with request ids and timestamp so the service team can look into that. This is valid from a semantics perspective, I'm not sure why the service doesn't accept it.

In the meantime, and as a workaround, you might want to issue another request which would be a client.Users["id"].Manager.Ref.DeleteAsync()

[mkomko]

I suggest you open a support ticket with request ids and timestamp so the service team can look into that. This is valid from a semantics perspective, I'm not sure why the service doesn't accept it.

Okay, thanks for your time! I'll open a ticket, but my experience in the past has not been great. I've never gotten to a point where a knowledgable person received my ticket.

In the meantime, and as a workaround, you might want to issue another request which would be a client.Users["id"].Manager.Ref.DeleteAsync()

That's what we're doing now and kind of led to this PR in the first place 😉

If you feel that this should be the end of this PR, feel free to close it. I stand by my opinion of this being a useful addition 😄.

[baywet]

Okay, thanks for your time! I'll open a ticket, but my experience in the past has not been great. I've never gotten to a point where a knowledgable person received my ticket.

We've received this feedback repeatedly at the division level, our CPX (customer experience) team has put a lot of energy into making sure the support and service teams are better equipped to solve customers issue. Hopefully you have a better experience this time.

I stand by my opinion of this being a useful addition

Yes, however the lack of additional customer demander and the source breaking nature of the change probably means it's a lower priority for us. Can I ask you create an issue about the lack of removal method please? We'll track it for a v2.

Closing.