New: Validate Cloud Validators #7154
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7154 +/- ##
==========================================
- Coverage 94.03% 94.03% -0.01%
==========================================
Files 172 172
Lines 12913 12959 +46
==========================================
+ Hits 12143 12186 +43
- Misses 770 773 +3
Continue to review full report at Codecov.
|
|
Good call, added in |
|
Needs to include new validator options |
…server into validateValidator
|
Why is a failed validation only logging an error (that can be easily overseen) and not throwing an error on server start? I assume that in usual cases, validation configuration is not dynamically defined but statically defined once on server start when Cloud Functions are loaded. So if this fails once, it will always fail, thus it can be predicted to fail, thus it could tell the developer at server start already that there is a misconfiguration. Given its expected use as security / access rights related feature, preventing to start the server seems more appropriate than just logging an error. We also don't just log but throw an error if a Parse Server option is incorrect, for example. That's another reason why I suggested that this PR should ideally use a generic validation logic, for unified incorrect config handling across Parse Server. Otherwise this gets increasingly confusing for developers, sometimes logging, sometimes throwing, sometimes silently failing gracefully. |
|
Well, thanks for reacting so fast in changing to throwing, but I actually wanted to start a discussion about it to find a solution. What's your opinion on that? |
|
Sorry @mtrezza I’d thought that I’d replied to you. I didn’t realise that you meant a class to validate the Parse Server options as well. I think that’s a good idea and will make it a lot easier to add new features. I’d be happy to work on that in this PR if that’s the course you’d like to take. I think throwing the error makes sense. The only concern is that it might break some configurations if they’ve accidentally mistyped a field. |
|
I agree that a generic config validation doesn't have to be in this PR.
Isn't mistyping one of the reasons why we'd want to throw? Can you give an example? |
|
I mean that I would break existing configurations that currently have a mistyped key. It would effectively be a “breaking change”. |
|
Introducing validation is not a breaking change as we do not assume misconfiguration as the basis reference to determine what constitutes a breaking change. In other words, a misconfiguration is a developer bug, to which the same considerations apply as to a repository bug, which is also not assumed to be a basis reference. Quite the contrary, especially security related bugs (by developer or repo) should be revealed as prominently as possible to fix them quickly. |
|
@dblythy Would you merge current master to get the tests to pass? |
|
@dblythy Do you think this PR is ready, or anything you want to add? |
I'm happy with it, thanks Manuel 😊 |
|
Great! |
|
🎉 This change has been released in version 5.0.0-beta.1 |
|
🎉 This change has been released in version 5.0.0 |
New Pull Request Checklist
Issue Description
Adds a cloud validator validator to make sure that cloud validators are formatted correctly.
Related issue: #7153
Approach
Validates when a new trigger is created.
TODOs before merging