Kong
refactor(DAO): introduce validate and fix check
DO NOT MERGE!
This PR causes a breaking change. We planed this for 4.0.
Review Note
Please check "Hide whitespace" when reviewing.
Summary
@flrgh:
- This has_common_protocol_with_service() validation function is a liar and does not actually check the service protocol.
- For both routes and services, the protocol checks happen before the input is populated with default values, so a plugin payload with an explicit { "protocols": [ "http" ] } will [correctly] result in a validation error, but a plugin payload with protocols omitted will be [erroneously] accepted.
We want validation to be after default values are populated.
I noticed that insert/update/upsert shares code. It seems reasonable to add a validation handler, which happens before a real DB operation, after the auto_field_process.
Another advantage of this method is that it won't break other uses. We could still override the insert/update/upsert method to do the validation.
Checklist
- [x] The Pull Request has tests (covered by original test)
- [x] There's an entry in the CHANGELOG
- [ ] N/A
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE
Full changelog
- Refactoring the DAO: adding a method named "validate", happen before strategy.insert/update/upsert.
- Fix the test. The original test relies on the ordering of checks(protocol check happens before other schema checks).
Behavior:
- For plugins attached to a service:
- Before the change, it needs to match at least one protocol of a route attached to the service if any;
- After the change, it needs to match the service's protocol
- For both: the check is done after populating the default values
Issue reference
Fix FTI-4436
This feels like small but potentially breaking change.
- we allow "invalid" configurations currently and then can be stored in database
- we make our validations more correct BUT restricting, which would make the previous database start to fail now
@hbagdi?
I agree with @bungle 's worries.
I think we can live with this bug till 4.0. We can choose to emit warnings only. This change only affects protocol checking. Only if we intentionally apply this new handler to a DAO entity will it affect its behavior (otherwise it remains the same behavior as before). So the idea of emitting warnings is feasible.
