Query Parameters are not working due to missing $ prefix in logic

[tommed]

As mentioned in #114, AddQueryParameters which sets the $search, $expand, $top, $skip etc. in github.com/microsoft/[email protected]/request_information.go is using reflection to name the parameters which you are adding to your URLs in all API calls (e.g. (m *UsersRequestBuilder) CreateGetRequestInformation(options *UsersRequestBuilderGetOptions)).

Unfortunately you need to inject some additional logic as the MS Graph query parameters require the $ ODATA prefix.

This means no API calls work as soon as you supply query parameters (so no paging, no filtering, no field fetching) otherwise you get an error like:

Request_BadRequest: Unrecognized query argument specified: 'top,skip'.: error status code received from the API

I believe you should be able to fix this inside this library - the logic in microsoft/kiota-abstractions-go isn't wrong, you just can't use the final results in this library because of MS graph API / OData requirements for the $ prefix.

[tommed]

Example of braking code:

var offset int32 = 0
var pageSize int32 = 1
res, err := c.graph.Users().Get(&users.UsersRequestBuilderGetOptions{
  QueryParameters: &users.UsersRequestBuilderGetQueryParameters{
    Select: models.AzureADSelectFields,
    Expand: models.AzureADExpandFields,
    Skip: &offset,
    Top:  &pageSize,
  },
})

[baywet]

Hi @tommed
Thanks for reaching out and for trying the Go SDK.
You are correct, we made some changes to the way special characters are handled in parameter names and it seems that during the last generation a misalignment happened.

msgraph-sdk-go/users/users_request_builder.go

Line 47 in 8bfa602

Select []string;

Should have a tag like this uriparametername:"%24select".

I've just triggered a new generation to try and remediate this issue.

[baywet]

Update: just merged the pull request that addresses the issue. Running go get github.com/microsoftgraph/[email protected] should resolve the issue for you.

[tommed]

I think this has broken something else now.. just collecting details, but are there not any unit tests to validate the builds before release?

[tommed]

Same code as above now breaks internally:

code = "InternalServerError" message = "Unable to find target address"

using v0.19.0

[baywet]

with the following snippet

        var offset int32 = 0
	var pageSize int32 = 1

	result, err := client.Users().Get(&users.UsersRequestBuilderGetOptions{
		QueryParameters: &users.UsersRequestBuilderGetQueryParameters{
		  Select: []string{"id", "displayName", "mail", "userPrincipalName"},
		  Expand: []string{"manager"},
		  Skip: &offset,
		  Top:  &pageSize,
		},
	})

I get the following error

{
    "error": {
        "code": "Request_BadRequest",
        "message": "'$skip' is not supported by the service.",
        "innerError": {
            "date": "2022-04-14T14:37:04",
            "request-id": "ac70be05-e206-4669-b63b-6b8e7325f981",
            "client-request-id": "13d4967d-5e0a-c8c9-c372-38355fae367d"
        }
    }
}

which is caused by the fact the service doesn't support skip and the fact the metadata we rely on to generate this SDK is inaccurate. I've logged an issue in the metadata repo so it can be addressed there.

Just removing the skip parameter makes the call work on my end.

The error you're getting seems like you're trying to query the users endpoint while being signed in with a personal Microsoft account, which is not supported by the service as indicated in the documentation. Please switch to an organizational account when signing in.

but are there not any unit tests to validate the builds before release?

As outlined above this latest issue is coming from the client code/service, not the SDK itself at this point. Unit testing generated code provides no value by itself. We do have hundreds of unit tests in the generator as well as in the core libraries. We're also planning to add Go support to the integration test infrastructure before releasing this SDK for general availability. Integration tests would have caught the missing tag in the query parameters names.

[tommed]

Sorry, the subsequent test took out the $skip (though it seems odd the API to list all users cannot be paged and only a maximum of 999?).

var pageSize int32 = 999
var expandFields = []string {
  "manager",
  "memberOf",
  "photo",
}
var selectFields = []string {
  "id",
  "businessPhones",
  "city",
  "companyName",
  "country",
  "department",
  "employeeId",
  "employeeType",
  "displayName",
  "givenName",
  "jobTitle",
  "mail",
  "officeLocation",
  "surname",
  "userPrincipalName",
  "accountEnabled",
}
apiResp, err := c.graph.Users().Get(&users.UsersRequestBuilderGetOptions{
  QueryParameters: &users.UsersRequestBuilderGetQueryParameters{
    Expand:  expandFields,
    Select:  selectFields,
    Orderby: []string{"userPrincipalName"},
    Top:     &pageSize,
    //Skip:   &offset, // <~ not supported?!?!
  },
})

Yields the error:

InternalServerError: Unable to find target address: error status code received from the API

[baywet]

Paging for those APIs can be achieved through the nextLink/skip token paradigm. Instead of implementing it yourself, you can leverage the page iterator the SDK provides

The error message you're getting still indicates you're signing in with a personal account. Can you double check this aspect please?

[tommed]

I'm using an Application's identity (Client ID and certificate/private key) via:

Error handling has been removed to keep the code succinct, but exists throughout in my copy.

// parse certs manually as SDK does not support ENCRYPTED PRIVATE KEY:
cert, key, err := parseCertificates(cfg.CertificatePEM, cfg.PrivateKeyPEM, cfg.CertificatePassword)

// build credential using client ID and cert against this tenant
cred, err := azidentity.NewClientCertificateCredential(cfg.TenantID, cfg.ClientID, cert, key, nil)

// now we need various adapters
authAdapter, err = a.NewAzureIdentityAuthenticationProvider(cred)
graphAdapter, err := msgraph.NewGraphRequestAdapter(authAdapter)

// now create graph client
graph := msgraph.NewGraphServiceClient(graphAdapter)

[tommed]

Paging for those APIs can be achieved through the nextLink/skip token paradigm. Instead of implementing it yourself, you can leverage the page iterator the SDK provides

The error message you're getting still indicates you're signing in with a personal account. Can you double check this aspect please?

Thanks. Re. your paging suggestion, I managed to get this working with the code from here as the documentation doesn't work atm, it shows graph.CreateMessageCollectionResponseFromDiscriminatorValue coming from "github.com/microsoftgraph/msgraph-sdk-go/models/microsoft/graph" whereas it should actually be models.CreateUserCollectionResponseFromDiscriminatorValue coming from models "github.com/microsoftgraph/msgraph-sdk-go-core".

Also, options.H should also be updated to options.Headers

[tommed]

%24top=999&%24orderby=userPrincipalName&%24select=id%2CbusinessPhones%2Ccity%2CcompanyName%2Ccountry%2Cdepartment%2CemployeeId%2CemployeeType%2CdisplayName%2CgivenName%2CjobTitle%2Cmail%2CofficeLocation%2Csurname%2CuserPrincipalName%2CaccountEnabled&%24expand=manager%2CmemberOf%2Cphoto

[tommed]

Interestingly, a call to c.graph.Applications().Get(nil) with Application.Read.All permission granted and approved works fine. So it's related specifically to the c.graph.Users().Get(...) call despite the error message suggesting this is some kind of addressing issue. Wonder if it's just a badly worded error?

Even when adding a $top parameter, c.graph.Applications().Get works.

[baywet]

removing photo and memberOf in the expand clause make the call work.

Photo

This property is effectively store on SharePoint or Exchange on the backend where the rest of the user object is in AAD if I remember right, even though it's not called out this way cross service expands don't always work in Microsoft Graph. The error message from the API should have been more explicit though.

MemberOf

Apparently identity services limit queries to one expand field, I wasn't aware of that.

So effectively the SDK is "doing the right thing", those are service side limitations.

[tommed]

So yes, the error message is pretty confusing.

So if I want to get company and memberOf for all matching results, it just can't do this?

[tommed]

Also, documents are still wrong as you're casting to the model and not a pointer to the model:

message := pageItem.(graph.Message)

// should be a pointer cast
if message, ok := pageItem.(*graph.Message); ok {
   // ...
}

[baywet]

I'm not sure, at this point the best course of actions for you would be to post in the Microsoft Graph identity Q&A forum the service team will be able to assist you.
When asking questions there, it's better to simply ask about the URL/request, not to mention any language.

For the sample, do you have a link? We've recently updated those (see #135), see the updated version https://github.com/microsoftgraph/msgraph-sdk-go#41-get-all-the-users-in-an-environment

(it now uses interfaces to support upcast)

[tommed]

In your link you are using models.CreateUserCollectionResponseFromDiscriminatorValue but you aren't casting to models.User anymore, you're casting to models.Userable?

The previous documentation you linked for me (above) case to models.Message etc. I tried to use models.User in my example but realised this didn't work because it's actually a *models.User (obviously to conform to the interface).

If the documentation is using the more abstract version, it will work too - just surprising!

Generally, if you don't mind me saying, this library seems to be incredibly complex and not at all golang-y. Seems like an awful lot of dependencies and time taken to build when it's a fairly straight forward odata http RESTful service. Would be nice to see a proper go-first implementation rather than something code-generated and similar across all languages regardless of their own language paradigms. Just a thought, feel free to close this ticket. Thanks for all the attention.

[baywet]

In your link you are using models.CreateUserCollectionResponseFromDiscriminatorValue but you aren't casting to models.User anymore, you're casting to models.Userable?

Correct. Is your concern about the User part of the method name and the Userable cast? This is expected by conventions.

If the documentation is using the more abstract version, it will work too - just surprising!

the public docs also have been updated, unless I'm missing something in this comment?

Generally, if you don't mind me saying, this library seems to be incredibly complex and not at all golang-y. Seems like an awful lot of dependencies and time taken to build when it's a fairly straight forward odata http RESTful service. Would be nice to see a proper go-first implementation rather than something code-generated and similar across all languages regardless of their own language paradigms

This kind of feedback is precious for us to "get it right". It's not specific enough though. Do you have specific examples about what doesn't feel idiomatic and what you'd expect instead?

[tommed]

I've written something instead internally which calls the msgraph api, and another package for specifics relating to Azure AD - so feel free to close this ticket. It was only half a day's work because the API is so simple - so appreciate that! 🙏 thanks

[meain]

This seems like a related issue. Not sure if it a problem with just the client though. I am trying to pull files from OneDrive with a select query. From the logs, this looks like the query that is being fired:

https://graph.microsoft.com/v1.0/drives/<drive-id>/root/microsoft.graph.delta()?$select=id%2C%40content.downloadUrl

This gives me just the id (plus @odata.type and @odata.etag), but the content.downloadUrl is not available. With the $ removed from $select it works as expected.