Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing to query for report #97

Closed
GoneUp opened this issue Mar 4, 2022 · 13 comments · Fixed by #310
Closed

Failing to query for report #97

GoneUp opened this issue Mar 4, 2022 · 13 comments · Fixed by #310
Assignees
Labels
bug Something isn't working fixed

Comments

@GoneUp
Copy link

GoneUp commented Mar 4, 2022

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

@ghost ghost added the Needs Triage 🔍 label Mar 4, 2022
@GoneUp GoneUp changed the title Fail to query for report Failing to query for report Mar 4, 2022
@baywet baywet self-assigned this Mar 4, 2022
@baywet baywet added Needs author feedback question Further information is requested and removed Needs Triage 🔍 labels Mar 4, 2022
@baywet
Copy link
Member

baywet commented Mar 4, 2022

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)

@ghost
Copy link

ghost commented Mar 9, 2022

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.

@GoneUp
Copy link
Author

GoneUp commented Mar 9, 2022

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.

@baywet
Copy link
Member

baywet commented Mar 10, 2022

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 baywet added bug Something isn't working blocked resolving this issue is blocked by an upstream dependency and removed question Further information is requested Needs Attention 👋 labels Mar 23, 2022
@baywet
Copy link
Member

baywet commented Apr 14, 2022

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
Copy link
Member

baywet commented Apr 19, 2022

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.

@darrelmiller
Copy link

darrelmiller commented Apr 19, 2022

 <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.

@baywet
Copy link
Member

baywet commented Apr 19, 2022

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

@darrelmiller
Copy link

@baywet That's probably the easiest path forward

@baywet
Copy link
Member

baywet commented Apr 19, 2022

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
Copy link
Member

baywet commented Jul 4, 2022

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
Copy link
Member

baywet commented Oct 19, 2022

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

@baywet baywet linked a pull request Nov 15, 2022 that will close this issue
@baywet
Copy link
Member

baywet commented Nov 15, 2022

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

@baywet baywet closed this as completed Nov 15, 2022
@baywet baywet added fixed and removed blocked resolving this issue is blocked by an upstream dependency labels Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants