Azure-sdk-for-go: graphrbac: Application homepage (signInUrl) cannot be cleared

[manicminer]

Bug Report

• github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac
• v42.1.0
• go version go1.14.2 darwin/amd64

• Trying to clear the Homepage property for an Application where it was previously set
• The property to be cleared (set to null in the application manifest)

Create an application with Homepage property set to some value

import "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac"

name := "TestApp"
homepage := "http://home.page"

properties := graphrbac.ApplicationCreateParameters{
    DisplayName: &name,
    Homepage: &homepage,
}

app, _ := client.Create(ctx, properties)

Then later try to clear the Homepage property

updateProperties := graphrbac.ApplicationUpdateParameters{
    Homepage: nil
}

app, _ := client.Patch(ctx, app.ObjectID, updateProperties)

The property is absent from the object in the PATCH request.

Try to set to an empty string

newHomepage := ""

updateProperties := graphrbac.ApplicationUpdateParameters{
    Homepage: &newHomepage,
}

app, _ := client.Patch(ctx, app.ObjectID, updateProperties)

and you get a 400 reply.

{
    "odata.error": {
        "code": "Request_BadRequest",
        "message": {
            "lang": "en",
            "value": "Invalid value specified for property 'homepage' of resource 'Application'."
        },
        "requestId": "80aa05e3-4fbf-4f85-a827-5319a4c9fa74",
        "date": "2020-06-16T02:14:31",
        "values": [{
            "item": "PropertyName",
            "value": "homepage"
        }, {
            "item": "PropertyErrorCode",
            "value": "InvalidLength"
        }]
    }
}

This property is nullable using the Azure Portal.

[ArcturusZhang]

Hi @manicminer thanks for this issue!

Due to the nature of http verb of PATCH, when you are leaving an attribute empty, the service would assume that you are not updating that attribute. In go SDK, when you assigned some property to nil, go SDK will not marshal that into the request body (this is how omitempty works) therefore the homepage would not change.

And the service must be validating the parameters it receives, therefore setting homepage to empty string would get an error (the service should be expecting this to be a http URL). Therefore I assume that you could not erase this by using the Patch function.

Have you tried if in Create function you leave the homepage empty, would the service fill this field for you when you get the application after creation complete?

BTW invoking someone from the service team to answer this business related question...

[msftbot[bot]]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @armleads-azure.

[manicminer]

Hi @ArcturusZhang, thanks for the explanation. With the Create function, leaving the homepage nil omits it from the payload and the service does indeed set the property to null. This makes it possible to create applications with this property disabled, but once it has been set there seems to be no way to unset it using the SDK?

Note that the Portal exposes this field in the Branding blade form, and if you clear the value and submit, it's set to null in the API request and updates the property to null as you'd expect (see screenshots) - so I believe this is an SDK issue.

[ArcturusZhang]

Hi @manicminer could you post a full request body from the portal?

I suppose this may have something to do with the omitempty in golang. omitempty will leave corresponding attribute in the result JSON empty when nil is assigned, rather than an explicit null value. I am not so sure that in a RESTful API, the service should regard null value attribute the same as those absent attributes in a PATCH request.

I am not quite familiar with this resource, therefore I did some experiments using the resource groups (resource groups have PATCH request). First I created a resource group by using the following body:

{
    "location": "eastus",
    "tags": {
        "test": "postman"
    }
}

And then I send a PATCH request with the following body expecting it to erase all tags for me:

{
    "tags": null
}

But unfortunately it does not work, the tags remain on the resource group.

Therefore I would like to know the full request and response on the portal to see where is the difference. Thanks a lot!

[manicminer]

@ArcturusZhang The request body is the same as the one that's visible in the screenshot.

{"id":"7aa0c6e5-03d1-4ca0-bd1b-c2bab37ee49d","signInUrl":null}

I captured another request where in addition to the Homepage URL (signInUrl) I also cleared the Terms of Service URL and Privacy Statement URL.

{"id":"1dd5b133-5faf-48e8-a621-75c2bbdff727","signInUrl":null,"informationalUrls":{"termsOfService":"","privacy":""}}

It seems to depend on the particular property being set, but for some properties, setting a value to null is the canonical means for clearing or unsetting it. As you can see, for signInUrl, a null value is the way to go.

I think you're right about omitempty being unhelpful here, though I'm not sure what the best approach would be. Ideally there should be a way to set a property value to null.

The relevant Portal blade:

[ArcturusZhang]

Yeah...
I searched some doc and am trying to get some documented information for how the service should behave in PATCH request when a field is absent or is explicitly assigned to null. Currently I could only say that this is an undefined behaviour - the service could choose whether to ignore those field assigned to null in PATCH or regard it as an erase.

For the resource group service, the service chose to ignore the null-valued field in PATCH, but in this service, maybe not. In fact, I currently do not have any idea that we could do on the SDK side for this (maybe remove the omitempty on some occasion? but
this may need some update on the swagger and code generator).

@jhendrixMSFT could you provide some insight on this?

[jhendrixMSFT]

Unfortunately this is a known limitation with our current design. As noted, the omitempty tag gets in the way here, preventing the sending of null across the wire to clear a value. We have some thoughts on how to fix it, however it's been relatively low priority given the small number of APIs that require this functionality.