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

13.0 signature type seb #38292

Open
wants to merge 4 commits into
base: 13.0
from

Conversation

@seb-odoo
Copy link
Contributor

commented Oct 9, 2019

@robodoo rebase-ff

related to #38026

@seb-odoo seb-odoo self-assigned this Oct 9, 2019
@seb-odoo seb-odoo added 13.0 ORM RD labels Oct 9, 2019
@robodoo robodoo added the seen 🙂 label Oct 9, 2019
@seb-odoo seb-odoo requested a review from rco-odoo Oct 9, 2019
odoo/fields.py Outdated Show resolved Hide resolved
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 10, 2019
Binary fields are supposed to be saved as binary in cache, and not as string
which is also supported as an input value.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 10, 2019
Before this commit, if `max_width` and `max_height` are 0, the image would not
be opened by Pillow at all (to gain a bit of CPU) however this was actually a
bad idea in this case because then we don't ensure the given value is actually a
valid image when we save it, only later at display we notice it and it crashes.

This optimization only made sense when `image_process` is called many times,
from the route displaying images every time a visitor is requesting an image,
for example. It doesn't really have an impact for one time operations such as
creating/writing.

So when size parameters are 0, which is the default, another parameter must be
passed to `image_process` to ensure image validity, and `verify_resolution` is
actually built for this, as it does the minimal amount of processing:
- loading the image
- making sure it is valid
- and ensuring the resolution is not completely crazy (`IMAGE_MAX_RESOLUTION`).
It doesn't alter the original image if no other operation was requested.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 10, 2019
- ensure a valid image was given or prevent to save it otherwise, instead of
  allowing any file, which would then crash at display
- restrict to a reasonable size

PR: odoo#38292
closes odoo#38026
@seb-odoo seb-odoo force-pushed the odoo-dev:13.0-signature-type-seb branch from b622439 to 9ea178d Oct 10, 2019
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 10, 2019
Before this commit, if `max_width` and `max_height` are 0, the image would not
be opened by Pillow at all (to gain a bit of CPU) however this was actually a
bad idea in this case because then we don't ensure the given value is actually a
valid image when we save it, only later at display we notice it and it crashes.

This optimization only made sense when `image_process` is called many times,
from the route displaying images every time a visitor is requesting an image,
for example. It doesn't really have an impact for one time operations such as
creating/writing.

So when size parameters are 0, which is the default, another parameter must be
passed to `image_process` to ensure image validity, and `verify_resolution` is
actually built for this, as it does the minimal amount of processing:
- loading the image
- making sure it is valid
- and ensuring the resolution is not completely crazy (`IMAGE_MAX_RESOLUTION`).
It doesn't alter the original image if no other operation was requested.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 10, 2019
- ensure a valid image was given or prevent to save it otherwise, instead of
  allowing any file, which would then crash at display
- restrict to a reasonable size

PR: odoo#38292
closes odoo#38026
@seb-odoo seb-odoo force-pushed the odoo-dev:13.0-signature-type-seb branch from 9ea178d to 8a7da4b Oct 10, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 10, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 11, 2019
@seb-odoo seb-odoo force-pushed the odoo-dev:13.0-signature-type-seb branch 2 times, most recently from 88af22f to 66b6915 Oct 14, 2019
@robodoo robodoo added the CI 🤖 label Oct 15, 2019
odoo/fields.py Outdated Show resolved Hide resolved
@rco-odoo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

@seb-odoo you can squash all commits.

seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
The compute methods are always expecting the real value and not the bin_size.

The solution is to always compute with `bin_size=False`, and then manually
compute the `bin_size` and set it on the cache of `bin_size=True`.

PR: odoo#38292

Co-authored-by Sébastien Theys <seb@odoo.com>
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
Binary fields are supposed to be saved as binary in cache, and not as string
which is also supported as an input value.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
Before this commit, if `max_width` and `max_height` are 0, the image would not
be opened by Pillow at all (to gain a bit of CPU) however this was actually a
bad idea in this case because then we don't ensure the given value is actually a
valid image when we save it, only later at display we notice it and it crashes.

This optimization only made sense when `image_process` is called many times,
from the route displaying images every time a visitor is requesting an image,
for example. It doesn't really have an impact for one time operations such as
creating/writing.

So when size parameters are 0, which is the default, another parameter must be
passed to `image_process` to ensure image validity, and `verify_resolution` is
actually built for this, as it does the minimal amount of processing:
- loading the image
- making sure it is valid
- and ensuring the resolution is not completely crazy (`IMAGE_MAX_RESOLUTION`).
It doesn't alter the original image if no other operation was requested.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
- ensure a valid image was given or prevent to save it otherwise, instead of
  allowing any file, which would then crash at display
- restrict to a reasonable size

PR: odoo#38292
closes odoo#38026
@seb-odoo seb-odoo force-pushed the odoo-dev:13.0-signature-type-seb branch from 33b40a1 to a3e54f5 Oct 15, 2019
@robodoo robodoo removed the CI 🤖 label Oct 15, 2019
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
The compute methods are always expecting the real value and not the bin_size.

The solution is to always compute with `bin_size=False`, and then manually
compute the `bin_size` and set it on the cache of `bin_size=True`.

PR: odoo#38292

Co-authored-by Sébastien Theys <seb@odoo.com>
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
Binary fields are supposed to be saved as binary in cache, and not as string
which is also supported as an input value.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
Before this commit, if `max_width` and `max_height` are 0, the image would not
be opened by Pillow at all (to gain a bit of CPU) however this was actually a
bad idea in this case because then we don't ensure the given value is actually a
valid image when we save it, only later at display we notice it and it crashes.

This optimization only made sense when `image_process` is called many times,
from the route displaying images every time a visitor is requesting an image,
for example. It doesn't really have an impact for one time operations such as
creating/writing.

So when size parameters are 0, which is the default, another parameter must be
passed to `image_process` to ensure image validity, and `verify_resolution` is
actually built for this, as it does the minimal amount of processing:
- loading the image
- making sure it is valid
- and ensuring the resolution is not completely crazy (`IMAGE_MAX_RESOLUTION`).
It doesn't alter the original image if no other operation was requested.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
- ensure a valid image was given or prevent to save it otherwise, instead of
  allowing any file, which would then crash at display
- restrict to a reasonable size

PR: odoo#38292
closes odoo#38026
@seb-odoo seb-odoo force-pushed the odoo-dev:13.0-signature-type-seb branch from a3e54f5 to c71a832 Oct 15, 2019
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
The compute methods are always expecting the real value and not the bin_size.

The solution is to always compute with `bin_size=False`, and then manually
compute the `bin_size` and set it on the cache of `bin_size=True`.

PR: odoo#38292

Co-authored-by Sébastien Theys <seb@odoo.com>
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
Binary fields are supposed to be saved as binary in cache, and not as string
which is also supported as an input value.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
Before this commit, if `max_width` and `max_height` are 0, the image would not
be opened by Pillow at all (to gain a bit of CPU) however this was actually a
bad idea in this case because then we don't ensure the given value is actually a
valid image when we save it, only later at display we notice it and it crashes.

This optimization only made sense when `image_process` is called many times,
from the route displaying images every time a visitor is requesting an image,
for example. It doesn't really have an impact for one time operations such as
creating/writing.

So when size parameters are 0, which is the default, another parameter must be
passed to `image_process` to ensure image validity, and `verify_resolution` is
actually built for this, as it does the minimal amount of processing:
- loading the image
- making sure it is valid
- and ensuring the resolution is not completely crazy (`IMAGE_MAX_RESOLUTION`).
It doesn't alter the original image if no other operation was requested.

PR: odoo#38292
seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Oct 15, 2019
- ensure a valid image was given or prevent to save it otherwise, instead of
  allowing any file, which would then crash at display
- restrict to a reasonable size

PR: odoo#38292
closes odoo#38026
@seb-odoo seb-odoo force-pushed the odoo-dev:13.0-signature-type-seb branch from c71a832 to 59712c1 Oct 15, 2019
@seb-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2019

Adapted PR following our discussion. The final diff of each commit looks really clean now. Can probably be merged after a final check tomorrow if runbot is ok.

rco-odoo and others added 4 commits Oct 15, 2019
The compute methods are always expecting the real value and not the bin_size.

The solution is to always compute with `bin_size=False`, and then manually
compute the `bin_size` and set it on the cache of `bin_size=True`.

PR: #38292

Co-authored-by Sébastien Theys <seb@odoo.com>
Binary fields are supposed to be saved as binary in cache, and not as string
which is also supported as an input value.

PR: #38292
Before this commit, if `max_width` and `max_height` are 0, the image would not
be opened by Pillow at all (to gain a bit of CPU) however this was actually a
bad idea in this case because then we don't ensure the given value is actually a
valid image when we save it, only later at display we notice it and it crashes.

This optimization only made sense when `image_process` is called many times,
from the route displaying images every time a visitor is requesting an image,
for example. It doesn't really have an impact for one time operations such as
creating/writing.

So when size parameters are 0, which is the default, another parameter must be
passed to `image_process` to ensure image validity, and `verify_resolution` is
actually built for this, as it does the minimal amount of processing:
- loading the image
- making sure it is valid
- and ensuring the resolution is not completely crazy (`IMAGE_MAX_RESOLUTION`).
It doesn't alter the original image if no other operation was requested.

PR: #38292
- ensure a valid image was given or prevent to save it otherwise, instead of
  allowing any file, which would then crash at display
- restrict to a reasonable size

PR: #38292
closes #38026
@seb-odoo seb-odoo force-pushed the odoo-dev:13.0-signature-type-seb branch from 59712c1 to dd5fc99 Oct 15, 2019
@seb-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2019

  • now also rebased to fix conflict on odoo/addons/test_new_api/ir.model.access.csv.
@robodoo robodoo added the CI 🤖 label Oct 15, 2019
@seb-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2019

Let's merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.