-
Notifications
You must be signed in to change notification settings - Fork 32
🐛 Fix Decimal serialization #6854
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 Decimal serialization #6854
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6854 +/- ##
==========================================
+ Coverage 88.33% 89.20% +0.87%
==========================================
Files 1553 1268 -285
Lines 61193 53074 -8119
Branches 2095 874 -1221
==========================================
- Hits 54055 47347 -6708
+ Misses 6806 5591 -1215
+ Partials 332 136 -196
Continue to review full report in Codecov by Sentry.
|
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.
This change affects some models in the web api schema. Can you please, cd web/server and
- increase minor version
make minor-version - update openapi specs
make openapi-specs
…/osparc-simcore into fix-decimal-serialization
bisgaard-itis
left a comment
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.
Can you please wait a bit with merging this one? This will break the api-server. It essentially reverts the changes I did in https://github.com/ITISFoundation/osparc-simcore/pull/6658/files in order to make the api-server as "backwards compatible as possible". I am confident your changes should go in. But it would be nice to have a strategy for how to handle this in the api-server before this breaks it. I will discuss with @pcrespov tomorrow how to handle this. Sorry for the inconvenience.
I am a bit scared by the fact that you don't see here that you are breaking backwards compatibility of the api-server. I think I will set up a hook so people see that they are breaking our interface |
pcrespov
left a comment
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.
@bisgaard-itis @giancarloromeo I will temporary block it to avoid accidental merging. Let me know if this is a problem for you! :-)
|
@pcrespov @bisgaard-itis The related PR (#6853) is already in. |
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.
OK, this one can go in for me. I will have to handle the backwards compatibility directly in the api-server. I am half way there and this should be quite easy to handle. @pcrespov I believe you can also approve this one
bisgaard-itis
left a comment
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.
Could you please also update the api-server's openapi.json in this PR?
cd services/api-server
make install-dev
make openapi.json
bisgaard-itis
left a comment
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 a lot for the effort.
pcrespov
left a comment
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.
Discussed with @bisgaard-itis . It does not affect backwards compatibility f the api-server
|



What do these changes do?
Pydatic v2 serializes
Decimals as strings to avoid loss of precision derived from conversion to float. This PR reverts the partial changes made during the migration.Related issue/s
How to test
Dev-ops checklist