-
Notifications
You must be signed in to change notification settings - Fork 449
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
fixed race conditions in the signup page #1023
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1023 +/- ##
===========================================
+ Coverage 92.90% 92.92% +0.02%
===========================================
Files 38 38
Lines 2073 2079 +6
===========================================
+ Hits 1926 1932 +6
Misses 147 147
|
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.
Hey @epicadk Have you tested what happens if you insert some record that will not cause integrity error, but some other form of error, e.g. DataError
if the data is too long? You're catching just integrity error and rollback only in that case, we need a more general approach. DBAPIError
or even Exception
may help there.
@NenadPantelic If the data is too long then I think we validation will not pass and it will return 400. I am not sure what other types or errors we'd want to handle. 😕 could you provide an example other than the data not matching the database constraints? |
Yes, in this case validators will solve the potential issue. I'm saying that we should protect the DAO layer, cause some DAO objects can be used in multiple controllers and also, we can get some unpredicted errors (transactional, persistence, etc.). In this case, I suppose it's fine on this occasion, cause validations are exhaustive and pretty much detailed and we work with the User model just from that controller, but this was more like a general comment, every layer that provides some service should be protected with its own mechanisms (like we have validations on the frontend, but we do them on the backend too and on the database). |
I suppose we could do that. However I don't know if that would be in the scope of this pr. Feel free to open another issue. 😃 |
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.
@epicadk can you please include some test cases to check if this works?
Done 😅 . |
Seems like github actions won't allow more than a single thread for execution. The test can't be added. Reverting the change. |
@epicadk I see the Github Workflow. There you mentioned Also, resolve the conflicts |
Doesn't matter I don't think github actions allows parallel execution of code of the tests also even if there are more than one threads it doesn't mean the code will run in parallel so it doesn't make sense to have tests for this. |
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.
LGTM 👍
Description
Fixes #1019
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Used to same program that caught this bug to verify if it has been solved or not.
Checklist:
Code/Quality Assurance Only