Dnn.Platform
Dnn.Platform copied to clipboard
dnnsoftware
Fix for #5291: Non-Integer Thumbnail Sizes Break API Calls
Fixes #5291
Summary
Implements a function ParseDimension to allow non-integer values for width and height, return a rounded value when decimals are used, and a DefaultDimension value when it is not a number.
Unit tests added.
This is a behavior change for sure, I think we need to hold/merge this with a 10.0.0 release. But option to discussion
@mitchelsellers it looks to me like this is a bugfix, previously it would return a 500 and now it returns a sensible dimension. My only concern is performance, but this method should not be hammered much and it has client cache enabled by default. Unless I am missing something...
@valadas I could see that, but we are going from "it must be an integer or it will error" to "it could be any type of number and we guess, or fall back"
@mitchelsellers I was initially against such code change (quoting internal ticket discussion):
I think the backend API is totally fine to throw HTTP 500 when the caller asks to get a thumbnail with non-integer width and height, because the resulting image has discrete dimensions. It’s simply a violation of the API contract to ask for an image with non-integer size.
but then got a guidance from my top management which I had to follow:
Why do I disagree with the rationale: the backend needs to be robust and always return a thumbnail. No thumbnail is worse than rounding up/down and breaking the layout, which would be broken anyway " for some customers when they heavily adjust the zoom and text-size on their browser."
Yeah, I agree that I would prefer for the calling code to send an integer there.
@v-karbovnichy this branch now has conflicts, and the maintainers haven't been given access to make changes to the branch, so we can't merge this PR. Please merge these changes with develop and then we can merge the PR. Thanks!
