ITISFoundation / osparc-simcore

🐼 osparc-simcore simulation framework
https://osparc.io
MIT License
44 stars 26 forks source link

πŸ› Fixes error while updated study with long description #5989

Closed pcrespov closed 1 week ago

pcrespov commented 1 week ago

What do these changes do?

Fixes error produced when the user updates a study with a long description. After these changes very long descriptions (>1000 chars) and titles (>200 chars) will be truncated without reporting an error. Note that these truncation only happens for input strings in the API. As a general rule all input data to the API should be constrained either with a hard error or silently (e.g. truncating).

As a side note, the front-end should have reacted better to a 4XX errors provided that is a client error (the description maximum length was surpassed). I believe changes like https://github.com/ITISFoundation/osparc-simcore/pull/5487 should help in this direction .

Related issue/s

How to test

Dev-ops checklist

None

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.4%. Comparing base (cafbf96) to head (7f97af2). Report is 293 commits behind head on master.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989/graphs/tree.svg?width=650&height=150&src=pr&token=h1rOE8q7ic&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation)](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation) ```diff @@ Coverage Diff @@ ## master #5989 +/- ## ========================================= + Coverage 84.5% 88.4% +3.8% ========================================= Files 10 1161 +1151 Lines 214 50767 +50553 Branches 25 562 +537 ========================================= + Hits 181 44902 +44721 - Misses 23 5739 +5716 - Partials 10 126 +116 ``` | [Flag](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation) | Coverage Ξ” | | |---|---|---| | [integrationtests](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation) | `64.7% <ΓΈ> (?)` | | | [unittests](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation) | `86.2% <100.0%> (+1.6%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation) | Coverage Ξ” | | |---|---|---| | [...c/models\_library/api\_schemas\_webserver/projects.py](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989?src=pr&el=tree&filepath=packages%2Fmodels-library%2Fsrc%2Fmodels_library%2Fapi_schemas_webserver%2Fprojects.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation#diff-cGFja2FnZXMvbW9kZWxzLWxpYnJhcnkvc3JjL21vZGVsc19saWJyYXJ5L2FwaV9zY2hlbWFzX3dlYnNlcnZlci9wcm9qZWN0cy5weQ==) | `100.0% <100.0%> (ΓΈ)` | | | [...s/models-library/src/models\_library/basic\_types.py](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989?src=pr&el=tree&filepath=packages%2Fmodels-library%2Fsrc%2Fmodels_library%2Fbasic_types.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation#diff-cGFja2FnZXMvbW9kZWxzLWxpYnJhcnkvc3JjL21vZGVsc19saWJyYXJ5L2Jhc2ljX3R5cGVzLnB5) | `98.4% <100.0%> (ΓΈ)` | | | [...core\_service\_api\_server/api/routes/studies\_jobs.py](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989?src=pr&el=tree&filepath=services%2Fapi-server%2Fsrc%2Fsimcore_service_api_server%2Fapi%2Froutes%2Fstudies_jobs.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation#diff-c2VydmljZXMvYXBpLXNlcnZlci9zcmMvc2ltY29yZV9zZXJ2aWNlX2FwaV9zZXJ2ZXIvYXBpL3JvdXRlcy9zdHVkaWVzX2pvYnMucHk=) | `92.3% <100.0%> (ΓΈ)` | | ... and [1150 files with indirect coverage changes](https://app.codecov.io/gh/ITISFoundation/osparc-simcore/pull/5989/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ITISFoundation)
pcrespov commented 1 week ago

Thanks for the quick fix!

Just a curiosity: you moved the issue from the osparc-issues repo to the osparc-simcore repo. Is there a particular reason?

@elisabettai I hope it is ok with you. I did it because: 1) it is a but in osparc-simcore code 2) it automatically gets in my list if it is in osparc-simcore

pcrespov commented 1 week ago

Instead of setting this limitation in the API, shouldn't it be part of the Project model?

@odeimaiz Some clarifications ~1. This constrained type is only used so far in the description field of service-metadata~ (sorry! mistake)!

  1. It is only used in the model used for the PATCH body, i.e. it is an input model, not an output. Therefore, once the data is in the system, I do not see a reason to truncate it again.

let me know if this is not clear or I misunderstood your question. We can talk offline

odeimaiz commented 1 week ago

Instead of setting this limitation in the API, shouldn't it be part of the Project model?

@odeimaiz Some clarifications ~1. This constrained type is only used so far in the description field of service-metadata~ (sorry! mistake)! 2. It is only used in the model used for the PATCH body, i.e. it is an input model, not an output. Therefore, once the data is in the system, I do not see a reason to truncate it again.

let me know if this is not clear or I misunderstood your question. We can talk offline

PATCH description and PUT project, isn't it? the PUT call is still used when the study is open.

pcrespov commented 1 week ago

project

Yes both. Sorry, for some reason I thought I used this in metadata descriptions

elisabettai commented 1 week ago

Thanks for the quick fix! Just a curiosity: you moved the issue from the osparc-issues repo to the osparc-simcore repo. Is there a particular reason?

@elisabettai I hope it is ok with you. I did it because:

1. it is a but in osparc-simcore code

2. it automatically gets in my list if it is in osparc-simcore

Thanks for explaining @pcrespov, it's not a big deal, but for the future I'd leave it in osparc-issues, for the following reasons:

  1. It is a bug that "real" (i.e. non-dev) users experience (even external users of osparc.io, lite, sim4life.io, etc.)
  2. We use the osparc-issues repo (in particular the special tags "Feedback" and "type:*" to provide metrics for NIH regarding user issues.