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

Fix for #5291: Non-Integer Thumbnail Sizes Break API Calls #5292

Merged
merged 4 commits into from
Dec 17, 2022
Merged

Fix for #5291: Non-Integer Thumbnail Sizes Break API Calls #5292

merged 4 commits into from
Dec 17, 2022

Conversation

v-karbovnichy
Copy link
Contributor

@v-karbovnichy v-karbovnichy commented Sep 12, 2022

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.

@dnfadmin
Copy link

dnfadmin commented Sep 12, 2022

CLA assistant check
All CLA requirements met.

@v-karbovnichy v-karbovnichy marked this pull request as ready for review September 12, 2022 15:49
@mitchelsellers
Copy link
Contributor

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

@valadas
Copy link
Contributor

valadas commented Sep 12, 2022

@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...

@mitchelsellers
Copy link
Contributor

@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"

@v-karbovnichy
Copy link
Contributor Author

@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."

@donker donker added this to the 9.11.1 milestone Sep 13, 2022
@valadas
Copy link
Contributor

valadas commented Sep 14, 2022

Yeah, I agree that I would prefer for the calling code to send an integer there.

@bdukes
Copy link
Contributor

bdukes commented Nov 8, 2022

@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!

@v-karbovnichy
Copy link
Contributor Author

@bdukes I had resolved the conflict, please proceed.

@bdukes bdukes merged commit 00be9a2 into dnnsoftware:develop Dec 17, 2022
zyhfish pushed a commit to zyhfish/Dnn.Platform that referenced this pull request Dec 29, 2022
…dnnsoftware#5292)

* DNN-61848 - Core Engineering Maintenance

* Update DefaultDimension to be 10 px

Co-authored-by: Ricardo Tejo <ricardo.tejo@trilogy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-Integer Thumbnail Sizes Break API Calls
6 participants