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/sanitize old data for usergroups.thumbnail #3498

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Nov 1, 2022

What do these changes do?

The thumbnail column in the groups table contains old data that does not validate as an url. I have found empty strings, or embedded images. From now on, any invalid thumbnail (i.e. a valud that does not match Optional[AnyUrl]) is automatically nullified.

This error happens while get_profile: The database has an old thumbnail that when parsed into ProfileGet a ValidationError is raised which is converted by the middle-ware into 500. Since WEBSERVER_DIAGNOSTICS= the error is logged in the backend:
image

why?: The problem is that groups resources are NOT validated (handlers are pretty old) when created but the Profile resource IS validated when read. That lead to a paradoxical situation in which you could write "invalid" group data but could not read it via the Profile. We can take care of that after the review #3499

@odeimaiz : I found entries with embedded images like "thumbnail": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAPgAAADMCAMAAABp5J ... . Do we really want to have embedded thumbnails or only URL? Does the front-end support both?

How to test

services/web/server/tests/unit/isolated/test_groups_models.py::test_sanitize_legacy_data

@pcrespov pcrespov marked this pull request as ready for review November 1, 2022 19:05
@pcrespov pcrespov self-assigned this Nov 1, 2022
@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Merging #3498 (1532147) into master (df68fb6) will decrease coverage by 0.4%.
The diff coverage is 100.0%.

❗ Current head 1532147 differs from pull request most recent head 2bc9315. Consider uploading reports for the commit 2bc9315 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3498      +/-   ##
=========================================
- Coverage    83.4%   82.9%    -0.5%     
=========================================
  Files         834     349     -485     
  Lines       35438   17823   -17615     
  Branches      748     133     -615     
=========================================
- Hits        29572   14791   -14781     
+ Misses       5676    2983    -2693     
+ Partials      190      49     -141     
Flag Coverage Δ
integrationtests 66.8% <55.5%> (-0.7%) ⬇️
unittests 82.2% <100.0%> (+1.7%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ver/src/simcore_service_webserver/groups_models.py 100.0% <100.0%> (ø)
...-v2/src/simcore_service_director_v2/cli/_client.py 0.0% <0.0%> (-100.0%) ⬇️
...v2/src/simcore_service_director_v2/cli/__init__.py 0.0% <0.0%> (-100.0%) ⬇️
...service_director_v2/cli/_close_and_save_service.py 0.0% <0.0%> (-100.0%) ⬇️
...or-v2/src/simcore_service_director_v2/cli/_core.py 0.0% <0.0%> (-87.5%) ⬇️
...ce_director_v2/modules/db/repositories/clusters.py 23.0% <0.0%> (-71.7%) ⬇️
...ervice_director_v2/api/routes/dynamic_scheduler.py 38.2% <0.0%> (-57.4%) ⬇️
...k/src/simcore_sdk/node_ports_v2/port_validation.py 47.9% <0.0%> (-48.0%) ⬇️
...simcore_service_director_v2/api/routes/clusters.py 57.6% <0.0%> (-41.1%) ⬇️
...e_service_director_v2/modules/projects_networks.py 38.2% <0.0%> (-37.1%) ⬇️
... and 550 more

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! thanks

@odeimaiz
Copy link
Member

odeimaiz commented Nov 2, 2022

Merci 👌.

Regarding the embedded image, I also saw it in prod AWS, I need to check if the frontend supports it.

@pcrespov
Copy link
Member Author

pcrespov commented Nov 2, 2022

Merci 👌.

Regarding the embedded image, I also saw it in prod AWS, I need to check if the frontend supports it.

@odeimaiz Please let me know because with this change it will be disabled. An alternative would to accept any kind of string for the thumbnail which would allow embedded images.

@sonarcloud
Copy link

sonarcloud bot commented Nov 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants