msgraph-sdk-go icon indicating copy to clipboard operation
msgraph-sdk-go copied to clipboard

Failing to query for report

Open GoneUp opened this issue 3 years ago • 12 comments

Hi,

I wanted to test if I could use this library for one usecase and stumbled over an error. After debugging it seems that the uri-template parser does not like the character ' which is present in the URI.

URI is set here and looks like this "{+baseurl}/reports/microsoft.graph.getOffice365ActiveUserDetail(period='{period}')"

Template parsing happens here

The error I get is this one Error getting the report: uritemplate:72:invalid literals

Code

func GetReport() {
	//auth
	cred, err := azidentity.NewClientSecretCredential(
		"x",
		"x",
		"x",
		nil,
	)

	if err != nil {
		fmt.Printf("Error creating credentials: %v\n", err)
	}

	auth, err := a.NewAzureIdentityAuthenticationProviderWithScopes(cred, []string{".default"})
	if err != nil {
		fmt.Printf("Error authentication provider: %v\n", err)
		return
	}

	adapter, err := msgraphsdk.NewGraphRequestAdapter(auth)
	if err != nil {
		fmt.Printf("Error creating adapter: %v\n", err)
		return
	}
	client := msgraphsdk.NewGraphServiceClient(adapter)


	period := "D7"
	result, err := client.Reports().GetOffice365ActiveUserDetailWithPeriod(&period).Get(nil)
	if err != nil {
		fmt.Printf("Error getting the report: %s\n", err)
		return
	}
	fmt.Printf("Found Drive : %v\n", result.GetContent())
}

BR Henry

GoneUp avatar Mar 04 '22 17:03 GoneUp

Hi @GoneUp , Thanks for trying the Go SDK and thanks for reaching out. There are a couple of layers to this.

First off, I believe the scope should be https://graph.microsoft.com/.default and not just .default.

The URI template library we use under the covers seems to have a bug, and I've opened an issue to engage with them.

Then, I believe the OpenAPI description is wrong at multiple levels

<Function Name="getOffice365ActiveUserDetail" IsBound="true" IsComposable="true">
        <Parameter Name="bindingParameter" Type="graph.reportRoot" />
        <Parameter Name="period" Type="Edm.String" Nullable="false" Unicode="false" />
        <ReturnType Type="graph.report" Nullable="false" />
      </Function>

Should give us at least two path items:

  • https://graph.microsoft.com/v1.0/reports/getOffice365ActiveUserDetail(period='{period}') (we have this)
  • https://graph.microsoft.com/v1.0/reports/getOffice365ActiveUserDetail(period='{period}')/content/$value

@darrelmiller could you confirm on the second endpoint please? I might be misinterpreting OData conventions.

This most likely has been fixed in the conversion library (complex properties expansion, composable functions expansion), but our generation pipeline is not taking advantage of those updates just yet. I have started putting the PRs together

For the fix to take effect, we'll need to inject a read restriction on the function as well so it expands the complex type properties.

Alternatively the OpenAPI description should describe a second operation for application/octect-stream (not sure how the conversion process would insert that?) and kiota, the generator behind that sdk, should add support for multiple operations as captured in this issue

All of that is fairly advanced internal works of how we generate the SDKs, and it's unlikely we'll be able to solve for all those things shortly.

As a work around, you can take any request builder that returns byte[] on a get method, and new it up passing the URL directly. I already described how to do that in #67 but this capability is impacted by another bug that should be solved by next week. (details in the thread)

baywet avatar Mar 04 '22 21:03 baywet

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

ghost avatar Mar 09 '22 08:03 ghost

Okay, thanks for taking a deep look into this.

I will try to implement the approach mentioned when the new release is there. Otherwise I'm also able to wait a few weeks and visit this topic again later.

GoneUp avatar Mar 09 '22 17:03 GoneUp

Update, the bug about using raw urls with request builders has been fixed. We're still working on supporting the different endpoint types in the generator however.

go get -u github.com/microsoftgraph/[email protected]

baywet avatar Mar 10 '22 18:03 baywet

update on the single quote issue, it took longer than expected because the actual RFC contained a mistake. After clearing this with the authors we're in the process of issuing an errata for the RFC and I've opened a PR on the uri template lib

I'm still waiting to hear from @darrelmiller on the URL conventions part (my first comment on this thread)

baywet avatar Apr 14 '22 18:04 baywet

update: the PR in the uri template library has been merged, and I put together #138 to address this aspect. This only thing that'll be left is the url conventions for the path to download the binary content.

baywet avatar Apr 19 '22 12:04 baywet

 <Function Name="getOffice365ActiveUserDetail" IsBound="true" IsComposable="true">
        <Parameter Name="bindingParameter" Type="graph.reportRoot" />
        <Parameter Name="period" Type="Edm.String" Nullable="false" Unicode="false" />
        <ReturnType Type="graph.report" Nullable="false" />
      </Function>

This metadata should give

https://graph.microsoft.com/v1.0/reports/getOffice365ActiveUserDetail(period='{period}') (we have this)
https://graph.microsoft.com/v1.0/reports/getOffice365ActiveUserDetail(period='{period}')/content

However, the metadata doesn't match the behavior of the APIs in most cases. In reality, the first URL returns a redirect and a CSV or JSON payload is returned directly. I have not found an API that actually implements the content path segment.

We have been asking the reporting team to update their report descriptions to this style to accurately reflect the API behavior:

<Function Name="getMailboxUsageStorage" IsBound="true">
  <Parameter Name="reportRoot" Type="graph.reportRoot"/>
  <Parameter Name="period" Type="Edm.String" Nullable="false" Unicode="false"/>
  <ReturnType Type="Edm.Stream" Nullable="false"/>
</Function>

However, not all of the report descriptions have been updated yet.

darrelmiller avatar Apr 19 '22 18:04 darrelmiller

thanks for chiming in @darrelmiller . Could we leverage XSLT to patch the ones that are still wrong for the time being and unlock customers?

baywet avatar Apr 19 '22 18:04 baywet

@baywet That's probably the easiest path forward

darrelmiller avatar Apr 19 '22 18:04 darrelmiller

thanks, logged this issue in the metadata repo so we "fix it in post". Once this is addressed the loop should be complete for this issue.

baywet avatar Apr 19 '22 19:04 baywet

To add to the conversation, the service team recently published this blog post. https://devblogs.microsoft.com/microsoft365dev/changes-to-microsoft-365-apps-usage-reports-api-in-microsoft-graph/

baywet avatar Jul 04 '22 12:07 baywet

update: the metadata has been fixed, we expect the fix to show up in the SDK by the end of next week.

baywet avatar Oct 19 '22 19:10 baywet

I believe this has been addressed via #310 and recent API changes. Closing. Don't hesitate to comment if something is still missing.

baywet avatar Nov 15 '22 20:11 baywet