-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Throw error when setting authData to null #6154
Throw error when setting authData to null #6154
Conversation
Looks good. Can you add a test case? |
Codecov Report
@@ Coverage Diff @@
## master #6154 +/- ##
==========================================
- Coverage 93.76% 93.75% -0.01%
==========================================
Files 166 166
Lines 11271 11291 +20
==========================================
+ Hits 10568 10586 +18
- Misses 703 705 +2
Continue to review full report at Codecov.
|
Actually it would be better to test this on a higher level instead of MongoTransform, so the test would be automatically done for mongoDB and Postgres, right? |
Yes, a REST call will work. I’ll do some additional testing on my end. |
Is there a way I can easily execute the test locally with Postgres? I only see the failing tests once I update the PR. |
Sent with GitHawk |
Yes here checks if user class or not
I don't believe it will throw again. This is a fail fast solution, doesn't matter what's in the DB. Can you write a failing test just in case? |
@dplewis The test you removed from MongoTransform would be the failing test. Can you try with that enabled? I believe it will fail. The issue was that in the transform, the If that doesn’t make much sense I can add the changes when I’m not on mobile. Regarding backwards compatibility, I believe it wouldn’t be worse than the current internal server exception if we throw there. Sent with GitHawk |
@mtrezza Oh I get what you are saying. My fix prevents setting authData. Your fix checks if authData field already exists and prevents it from overriding. |
@mtrezza I added back your changes and wrote more tests. |
it is forbidden to add a custom field name beginning with `_`, so if the object is not `_User` , the transform should throw
@dplewis I think we are getting close :) |
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 looks good.
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.
(fighting with the review feature)
@mtrezza Nice working with you, we should do this again sometime. 👍 |
@dplewis I agree, was my pleasure. |
* added ignore authData field * add fix for Postgres * add test for mongoDB * add test login with provider despite invalid authData * removed fit * fixed ignoring authData in postgres * Fix postgres test * Throw error instead of ignore * improve tests * Add mongo test * allow authData when not user class * fix tests * more tests * add condition to synthesize authData field only in _User class it is forbidden to add a custom field name beginning with `_`, so if the object is not `_User` , the transform should throw * add warning log when ignoring invalid `authData` in `_User` * add test to throw when custom field begins with underscore
Fixed
authData
field in DB overwriting synthesizedauthData
field that contains values of_auth_data_*
fields.Fixed only for mongoDB & Postgres.