Skip to content

Conversation

m1no
Copy link
Contributor

@m1no m1no commented Sep 24, 2025

Description

This PR fixes the issue as discussed in #13414. Multiple ideas were explored to fix this:

Exception Handler

Catching the exception would be a hotfix for this particular API, but as it is a defined API in multiple API calls of the Confluence API doc, a robust solution was preferred.

Content-Length Check

The response header Content-Length with a value of 0 should be an excellent indicator of an empty body, but unfortunately, this header is unavailable in those requests due to CORS restrictions.

Checklist

I have (if applicable):

  • [ N/A ] filed a contributor agreement.
  • referenced the GitHub issue this PR closes
  • [ N/A ] updated the appropriate changelog in the PR
  • [ N/A ] ensured the present test suite passes
  • [ N/A ] added new tests
  • [ N/A ] created a separate documentation PR in Quarto's website repo and linked it to this PR

It should be generally acknowledged that
returning an empty body for context-type
application/json is not a good practice
but some Confluence API endpoints do this
and sadly the chance of Atlassian changing
this is low so we have to deal with it.
@posit-snyk-bot
Copy link
Collaborator

posit-snyk-bot commented Sep 24, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

@m1no m1no marked this pull request as ready for review September 25, 2025 01:01
@cscheid cscheid self-requested a review September 25, 2025 13:08
Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

}

private handleResponse<T>(response: Response) {
private async handleResponse<T>(response: Response) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case anyone else looks at this: the change to async here changes the type of handleResponse. That means the handleResponse call sites change as well, but inspecting the code here shows that the only two call sites are already async, and call handleResponse in tail position. That means those calls do not need an additional await.

@cscheid cscheid merged commit 4eb8abf into quarto-dev:main Sep 25, 2025
51 checks passed
@cderv
Copy link
Collaborator

cderv commented Sep 25, 2025

Thanks @m1no !

@m1no m1no deleted the bugfix/issue-13414 branch September 25, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants