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

No HTTP status code information returned if response body is empty #151

Closed
ashmrtn opened this issue Feb 8, 2024 · 8 comments · Fixed by #152
Closed

No HTTP status code information returned if response body is empty #151

ashmrtn opened this issue Feb 8, 2024 · 8 comments · Fixed by #152
Assignees
Labels
bug Something isn't working Needs: Attention 👋 Standard GitHub label Issue caused by core project dependency modules or library

Comments

@ashmrtn
Copy link
Contributor

ashmrtn commented Feb 8, 2024

My team and I have discovered some resources that take too long to return data for certain page sizes. These resources consistently result in 503 errors with no body content being sent back. The retries in the default graph client don't get us passed the 503s either. When using the kiota-serialization-json-go library and other graph SDK libraries to communicate with the Graph servers we only get an error of "content is empty" back from graph SDK in these situations

This is problematic because we lose information about the HTTP status code. For example, we've found that if we see consistent 503s with no content returned we can reduce the requested page size to make progress. This strategy would probably not make sense if the status code was a 4xx error though

Would it be possible to update the graph SDK (I'm unsure which repo the update would be in) to somehow return the HTTP status code? We've worked around the issue for the moment by adding our own middleware to the graph client and explicitly checking for no content and returning an error, but that feels a bit heavy handed

As an additional note, depending on which graph SDK repo the fix is made in, other repos may need updating as well. For example, all the serialization libraries contain the line errors.New("content is empty") like I linked above

@baywet baywet transferred this issue from microsoft/kiota-serialization-json-go Feb 8, 2024
@baywet
Copy link
Member

baywet commented Feb 8, 2024

Hi @ashmrtn
Thanks for using kiota and for reporting this.
I think this is because the request implementation for ruby is missing a safeguard.
See the implementation in dotnet and compare it with the ruby one

Is this something you'd be willing to submit a pull request for?

@baywet baywet added bug Something isn't working Standard GitHub label Issue caused by core project dependency modules or library Needs: Author Feedback labels Feb 8, 2024
@baywet baywet transferred this issue from microsoft/kiota-http-ruby Feb 8, 2024
@baywet
Copy link
Member

baywet commented Feb 8, 2024

Sorry, I got confused with another issue for a moment, transferred this issue again.
Here is the problematic line in go

@ashmrtn
Copy link
Contributor Author

ashmrtn commented Feb 8, 2024

No worries, keeping everything in order between the different repos is certainly a task in itself. Thanks for moving the issue to the proper place

Unfortunately, I don't have the time right now to get a PR up for this. Looking a the golang library, it seems like a few more changes beyond the empty content check would be required since just returning nil without an error from getRootParseNode would result in the caller ultimately getting back no Parsable and no error

Ideally the caller should get back something that allows inspecting the HTTP status code. I don't believe kiota-http-go really has the framework to handle that at the moment since it relies on parsing odata errors sent back as response body content

@baywet
Copy link
Member

baywet commented Feb 8, 2024

That's almost the case. Once get root parse node returns nil on a unsuccessful status code, it'd in fact go here
and return a standard api error, which has a field for the status code and other things.
With that additional detail, would you be willing to submit a pull request to address the issue? (time allowing)

@ashmrtn
Copy link
Contributor Author

ashmrtn commented Feb 8, 2024

see #152

@baywet
Copy link
Member

baywet commented Feb 28, 2024

Hi @ashmrtn
Is this still something you're looking to fix with a pull request?

@ashmrtn
Copy link
Contributor Author

ashmrtn commented Feb 28, 2024

ah, sorry, this sort of fell off my radar. I think it was fixed by #152 and can be closed?

@baywet
Copy link
Member

baywet commented Feb 28, 2024

It does, sorry, I go confused reviewing a bunch of issues. I think we forgot to close this issue here, closing.

@baywet baywet closed this as completed Feb 28, 2024
@baywet baywet linked a pull request Feb 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Needs: Attention 👋 Standard GitHub label Issue caused by core project dependency modules or library
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants