feat(schema) friendlier behavior for at_least_one_of #3364

hishamhm · 2018-04-02T20:02:18Z

Make the behavior of the validation of routes friendlier:

Without this patch, if a user performs a PATCH request without giving explicit values to all three of methods, hosts or paths, we reject the request because we need to cross-check all three fields to ensure that at least one of those is not-null. If only one of those
is to be set, the user needs to explicitly set the other two to null in the request.

With this patch, if a user performs a PATCH request giving one non-empty value for at least one of methods, hosts or paths, we now accept it even if the other fields are missing, because this request cannot put these fields in an invalid state.

This was implementing in a very simple way, by adding a new internal property to entity_checkers, called run_with_missing_fields. Currently, the only entity checker that can run with missing fields is at_least_one_of, which is used by the Route entity.

This PR includes a "progression" test ( :) ). Note that this causes a couple of error messages to change, but I don't this should be considered a serious breaking change (especially since users are way less likely to bump into these errors with this patch).

Closes #3361.

thibaultcha · 2018-04-03T21:46:57Z

spec/02-integration/000-new-dao/02-db_core_entities_spec.lua

@@ -395,6 +395,26 @@ for _, strategy in helpers.each_strategy() do
          assert.same(route, new_route)
        end)

+        it("accepts a partial update to routing criteria when not null", function()


we can make this test case name more explicit for our future selves I think: "when at least one of the required fields it not null"

thibaultcha · 2018-04-03T21:49:45Z

spec/02-integration/000-new-dao/02-db_core_entities_spec.lua

+              ["@entity"] = {
+                "at least one of 'methods', 'hosts' or 'paths' must be non-empty",
+              }
+            },


It seems like this test becomes the same one as the above "fails trying to unset an interdependent field by itself":

we create a valid entity

we send an update with all included "semi-required" fields set to null

It seems like it becomes "fails trying to unset many interdependent field by themselves", which feels redundant with the previous test? It only differs in what the original entity has prior to the update, which should not change the result of the test because it focuses on the update operation.

The above test also deserves a comment that I will add here for brevity: it creates an entity with both hosts and methods, and sends an update nullifying methods. But the received error would be hard to swallow as a user: at least one of 'methods', 'hosts' or 'paths' must be non-empty. Yes, that is still the case. But the user does not know we do not do a read-before-write when updating this entity. It might be worth returning:

{ ["@entity"] = "at least one of '', '', '' must be non-empty when updating", methods = "field required for entity check when updating", hosts = "field required for entity check when updating", paths = "field required for entity check when updating", }

In other words, I think the behavior would be:

In insertion

always require at least one of the three

error out with the "at least one of..."

In update

if one of the required values is nil or null, require a non-null value

if all values are nil or null, error out with the more descriptive "field required for entity check when updating" for the user to be less confused

Remove the existing "field required for entity check" error seems it seems it comes unused? Or update it to the above, more descriptive, update-aware message

This ended up being a bit longer than I thought, but I hope it makes sense.

I definitely appreciate the thought given to the matter!

I modified the behavior to match what you described (removing the existing "field required for entity check" error in those cases, since the overall entity message now does a better job at describing what's going on). I also renamed the test, your description is better.

I got a bit confused about the redundancy of the tests -- I added it because the existing ones didn't cover the behavior I wanted to change. Perhaps it now covers both the behavior I wanted to test and that of another test? In other words, which particular test are you asking to be removed?

Make the behavior of the validation of routes friendlier: Without this patch, if a user performs a PATCH request without giving explicit values to all three of `methods`, `hosts` or `paths`, we reject the request because we need to cross-check all three fields to ensure that at least one of those is not-null. If only one of those is to be set, the user needs to explicitly set the other two to `null` in the request. With this patch, if a user performs a PATCH request giving one non-empty value for at least one of `methods`, `hosts` or `paths`, we now accept it even if the other fields are missing, because this request cannot put these fields in an invalid state. This was implementing in a very simple way, by adding a new internal property to `entity_checkers`, called `run_with_missing_fields`. Currently, the only entity checker that can run with missing fields is `at_least_one_of`, which is used by the Route entity.

Makes it easier to customize them, and in the future internationalize them.

Monkey-patch some error messages to make it clearer why they apply during an update. This is a bit ugly, but it avoids propagating update-awareness all the way down to the entity checkers (which would otherwise defeat the whole purpose of the abstracted entity checker mechanism, created because we don't want low-level logic to be dependent on `is_update` flags, which in the past led to relevant validations being overly skipped during updates).

thibaultcha · 2018-04-05T01:51:13Z

kong/db/schema/init.lua

@@ -64,6 +64,9 @@ local validation_errors = {
  ENTITY_CHECK              = "failed entity check: %s(%s)",
  ENTITY_CHECK_N_FIELDS     = "entity check requires %d fields",
  CHECK                     = "entity check failed",
+  CONDITIONAL               = "failed conditional validation",
+  AT_LEAST_ONE_OF           = "at least one of these fields must be non-empty: %s",
+  ONLY_ONE_OF               = "only one of these fields must be non-empty: %s",


I am slightly confused because I am not seeing ONLY_ONE_OF being used anywhere, and CONDITIONAL being tested nowhere I think? 😨 Was IF_THEN missing then?
I am afraid I am either missing something, or that this was your intent? I don't know. Could you help clarifying this for me? I am not necessarily seeing this as a blocker, but rather confused and looking for what your intent here is with those fields, and what that means with regards to the maintainability of the schema validation. Or if this is future errors, or anything else really.

they are used here: https://github.com/Kong/kong/pull/3364/files#diff-7135c27fc8fde710af0a85f4dc622d13R757

IF_THEN was indeed missing, but it mostly harmless because there's a fallback "failed entity check: %s(%s)" message (which adds further data). The idea is that entity checkers (added by us or in the future by others) will always produce messages, whether they return a custom message, a default message, or just a boolean.

(and just to clarify, the conditional entity checker is tested)

Oh I see, it is tested but does not have an assertion for its error message content. Alright, good enough it seems unrelated to the scope of this PR anyway.

thibaultcha · 2018-04-05T01:53:36Z

spec/02-integration/000-new-dao/02-db_core_entities_spec.lua

@@ -526,7 +551,7 @@ for _, strategy in helpers.each_strategy() do
          setup(function()
            assert(db:truncate())

-            for i = 1, 2000 do
+            for i = 1, 1002 do


👍 a good move

thibaultcha · 2018-04-05T02:01:13Z

spec/02-integration/000-new-dao/02-db_core_entities_spec.lua

+              ["@entity"] = {
+                "when updating, at least one of these fields must be non-empty: 'methods', 'hosts', 'paths'",
+              }
+            },


Moving the previous conversation here, but not considering it a blocker either (bigger fish to fry):

What I was saying is that it seems that both this test and the one at L.350 look alike:

They both create a Service with n amount of semi-required fields

They both issue an update which sets those n semi-required fields to null

This seems to be a case of "1 feature breaks and now N tests are failing" because they were testing the same thing.

Maybe you want to test both one and many semi-optional fields being unset, in which case maybe those names can help:

fails trying to unset an interdependent field by itself

fails trying to unset several interdependent fields at once

Or I am missing something else that differentiates those tests.

Again, only clarifying my previous point, I think the main concern I had with this PR personally was the lack of "when updating ..." context in the error message, which is now addressed.

Oh, I see.

These two tests as different because they document different aspects of system behavior:

the test at 350 starts with hosts[]=example.com methods[]=GET and tries to update with methods=null. A user could reasonably expect this to work, but it doesn't because we don't do read-before-write. This test documents this.

the test at 418 starts with hosts[]=example.com methods[]=GET and tries to update with hosts=null methods=null. A user could not reasonably expect this to work because it would break the constraint. This test verifies that indeed it does not work.

So, the second test fails for the same reason of the first test, but in the future the system behavior could be changed so that the first test becomes o a success-test but the second will still need to be a fail-test.

Ok, that is what I thought (that they currently test the same thing), lgtm if intentional then. I think I will just move the tests closer to each other and name them similarly then.

Thinking of sensible names to describe that difference, how about

fails if all routing criteria explicitly given in an update are null

fails if an update would cause all routing criteria to be null

thibaultcha · 2018-04-05T18:03:33Z

Alright, merged!

hishamhm mentioned this pull request Apr 2, 2018

Updating a route requires path, host, method to be set #3361

Closed

thibaultcha reviewed Apr 3, 2018

View reviewed changes

hishamhm force-pushed the feat/friendlier-entity-check branch from f72ff8d to 9496d2f Compare April 4, 2018 13:31

hishamhm added 3 commits April 4, 2018 11:12

feat(schema) de-hardcode entity checker error messages

efc356b

Makes it easier to customize them, and in the future internationalize them.

tests(dao) speed up pagination test

069e13e

hishamhm force-pushed the feat/friendlier-entity-check branch from 9496d2f to 069e13e Compare April 4, 2018 14:14

thibaultcha reviewed Apr 5, 2018

View reviewed changes

thibaultcha closed this Apr 5, 2018

thibaultcha deleted the feat/friendlier-entity-check branch April 5, 2018 18:03

bungle mentioned this pull request May 2, 2018

PATCH to routes requires hosts, methods and paths to be supplied #3431

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(schema) friendlier behavior for at_least_one_of #3364

feat(schema) friendlier behavior for at_least_one_of #3364

hishamhm commented Apr 2, 2018 •

edited

Loading

thibaultcha Apr 3, 2018

thibaultcha Apr 3, 2018

hishamhm Apr 4, 2018

thibaultcha Apr 5, 2018

hishamhm Apr 5, 2018

hishamhm Apr 5, 2018

thibaultcha Apr 5, 2018

thibaultcha Apr 5, 2018

thibaultcha Apr 5, 2018

hishamhm Apr 5, 2018

thibaultcha Apr 5, 2018

hishamhm Apr 5, 2018

thibaultcha commented Apr 5, 2018

feat(schema) friendlier behavior for at_least_one_of #3364

feat(schema) friendlier behavior for at_least_one_of #3364

Conversation

hishamhm commented Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Apr 5, 2018

hishamhm commented Apr 2, 2018 •

edited

Loading