response body from image+url request for better error reporting #2334#2350
Conversation
hvbtup
left a comment
There was a problem hiding this comment.
I don't know if this routine can also be called for other types of URLs, eg file URLs.
I think the error body "No HTTP-Protocol" is useless and even misleading in this case.
extractErrorBody should only be called for HTTP protocols.
|
@hvbtup valid point, i changed the pull-request |
|
This seems fine. Could you please share what the resulting error looks like? |
@mithrandirii Can you please respond? |
|
Moved to 4.24 |
|
Closed. Author @mithrandirii does not respond. |
|
@wimjongman sorry to not respond. i was off for a long time. the email from github reminded me. |
|
the log message without the change is: and with the change: the string "{"success":false,"message":"logo not found"}" is the html body of a example http image resource |
|
to get this working correctly i had to add two futher lines. i added it to my fork, but i don't know how to update the pr. maybe it only works for open pull-requests |
|
Amend the commit and force push it. There is one day left to close off the 4.24 release |
|
And be sure to rebase in master or the build will definitely fail. |
|
big thx for reopening the pr! if i look to files changed tab all commits are included. one check was cancelled: build CI but i don't understand why! soory, what do i need to do? |
|
@copilot review security implications. |
|
This commit chain is kind of a mess now. I don’t want to merge it in this form. If you’re not sure how to squash a single commit rebased on master, then maybe check out master, pull, and create a new branch that you push to your fork for a new pull request. I can have a look tomorrow, but it’s too late in the day for me. |
|
ok, i think i have it now. thx again for the support! |
There was a problem hiding this comment.
Looks okay, though for me testing with a broken link, the error stream does not contain anything useful:
More of concern is that if the connection isn't a URL connection, for which disconnect is called, the stream itself is not closed. I imagine disconnect closes it, but that's not called in all cases. I think that should be better guarded.
Better safe than sorry:
try {
connection = url.openConnection();
try (InputStream in = connection.getInputStream()) {
inBytes = getByteArrayFromInputStream(in);
cache.put(url, inBytes);
}
} catch (IOException e) {
There was a problem hiding this comment.
Pull request overview
Enhances BIRT report engine resource loading diagnostics by attaching HTTP response bodies (when available) to warning logs for failed URL-based resource retrievals, improving troubleshooting for dynamic image URL failures (Issue #2334).
Changes:
- Switches URL resource retrieval from
url.openStream()to an explicitURLConnectionto enable access to HTTP error streams. - Logs failures via a
LogRecordto preserve i18n message formatting while optionally attaching a throwable containing response-body details. - Adds helper logic to extract and truncate HTTP error response bodies.
|
i implemented changerequests from @merks and copilot. the resulting message looks like this now: |
|
@mithrandirii please resolve the conversations that you have implemented. |
|
And please amend the commit and force push it, because we again have 3 commits. We don't want the intermediate stages commit to the repository and we don't want to use squash and merge because it mess up the committer email to be some github bot thing. So this should be a single commit... |
|
ok, @wimjongman: resolved the conversations and @merks force-pushed to one commit |
|
I assume we are comfortable merging this before a final milestone build today? |
Yes, please go ahead. |
|
big thx to all for the support and the merge before the release |
|
A nightly build has been twigged and when that''s done I will trigger a milestone build which is the release candidate for tomorrow. |
|
Thanks Ed @merks, and thank you for the patch @mithrandirii. |
Added response body to failed responses in ResourceLocatorWrapper #2334