-
Notifications
You must be signed in to change notification settings - Fork 27
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
🐛 Fixes error while updated study with long description #5989
🐛 Fixes error while updated study with long description #5989
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of setting this limitation in the API, shouldn't it be part of the Project model?
@elisabettai I hope it is ok with you. I did it because:
|
@odeimaiz Some clarifications 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. |
Yes both. Sorry, for some reason I thought I used this in metadata descriptions |
Thanks for explaining @pcrespov, it's not a big deal, but for the future I'd leave it in
|
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 #5487 should help in this direction .
Related issue/s
How to test
-k test_project_patch_truncates_description
Dev-ops checklist
None