Handle HTTP 200 responses with no image for coverage task when failOnError enabled#3803
Conversation
ebc2ce1 to
4162205
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where HTTP 200 responses containing non-image content were not properly handled when failOnError is enabled for tiled map layers. Previously, these responses would only generate a warning log and continue processing, but now they properly throw an exception when failOnError is true.
- Enhanced error handling in
CoverageTaskto throw exceptions for invalid image responses whenfailOnErroris enabled - Added comprehensive test coverage for both WMS and tiled WMS scenarios with 200 responses containing non-image content
- Created test data files to support the new test scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| CoverageTask.java | Modified to throw RuntimeException when failOnError is true and response contains non-image data |
| CreateMapPagesFailOnErrorProcessorTest.java | Added new test methods and mock handlers to verify proper error handling for non-image 200 responses |
| requestData2.json | Test configuration for tiled WMS layer with failOnError enabled |
| requestData3.json | Test configuration for regular WMS layer with failOnError enabled |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
core/src/main/java/org/mapfish/print/map/tiled/CoverageTask.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/mapfish/print/processor/map/CreateMapPagesFailOnErrorProcessorTest.java
Outdated
Show resolved
Hide resolved
9235ebf to
3e1f4bd
Compare
|
You should run |
b4c811d to
94e1ba9
Compare
|
@sebr72 for me that's looks good. Can you take a second look and merge :-) |
|
Hello @sebr72 , have you got some time to check my PR ? Thank you ! |
|
Hello @sebr72 little reminder about this PR, if you have some times. Have a good day ! |
core/src/main/java/org/mapfish/print/map/tiled/CoverageTask.java
Outdated
Show resolved
Hide resolved
sebr72
left a comment
There was a problem hiding this comment.
Fix is good. Thanks.
One change to clarify or revert.
0ff2ad0 to
8ef0cd6
Compare
sebr72
left a comment
There was a problem hiding this comment.
Thanks.
I added the requested change in the comments
8ef0cd6 to
b075180
Compare
|
@sbrunner do you know when you will release a new version of mapfish ? |
|
Before creating version 4.0, we want to merge this pull request: #3827 What's do you prefer? You have some others needed for the 4.0? |
|
@sbrunner Yes if you can backport it on 3.33 , it would be great thank you. |
When using some tiled request that use coverage task, and fail on error is activated, no error occured on 200 HTTP response that does not send an actual image. Just a warn log is displayed and the generation is done.
I have fixed it. And added some UT.