[wp-trac] [WordPress Trac] #56494: REST API: Fix and test rest_default_additional_properties_to_false

[WordPress Trac]

#56494: REST API: Fix and test rest_default_additional_properties_to_false
--------------------------------------+------------------------------
 Reporter:  anna.bansaghi             |       Owner:  (none)
     Type:  defect (bug)              |      Status:  new
 Priority:  normal                    |   Milestone:  Awaiting Review
Component:  REST API                  |     Version:  6.0.2
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:
--------------------------------------+------------------------------

Comment (by anna.bansaghi):

 Hi @TimothyBJacobs, thanks for the review, and raising the need of
 discussion of passing the `$types` parameter.


 I was thinking about it and experimenting with the function a lot, and
 finally I came up with two possible solutions.

 It became a bit long write-up, but it helped me to organize the
 information around MR Trees for this function.

 = Insignificant changes

 At the beginning I would like to mention the least important changes:
 - moved the `array` section before the `object`
 - that moved the "Task" at the very end of the function


 Now the code can be split into 3 parts from "bigger" to "smaller" data
 structure:
 - recursive call on forest
 - recursive call on tree
 - performing the "Task" on the current node

 = Interesting changes


 The schema traversal can be implemented in two ways (hence the two
 solutions), depending on how we define the schema being "object".

 ==== Object definition

 The schema is an object if:
 - has `type ⊃ 'object'`

 Or we can say more permissive that if:
 - has `type ⊃ 'object'`, or
 - has `properties` keyword set, or
 - has `patternProperties` keyword set, or
 - has `additionalProperties` keyword set with `array` value

 Similarly a schema is an array if:
 - has `type ⊃ 'array'`, or
 - has `items` keyword set (not supporting the `tuple` form)


 ==== "Task" to be performed

 In this sense the "Task" can be implemented in two ways:

 {{{#!php
 <?php
 if ( ! isset( $schema['additionalProperties'] ) && in_array( 'object',
 $types, true ) ) {

     $schema['additionalProperties'] = false;
 }
 }}}


 or


 {{{#!php
 <?php
 if ( ! isset( $schema['additionalProperties'] ) &&
     ( in_array( 'object', $types, true ) ||
         isset( $schema['properties'] ) ||
         isset( $schema['patternProperties'] ) ) ) {

     $schema['additionalProperties'] = false;
 }
 }}}


 ==== Solution 1

 It is more similar to the original function, uses the first object
 definition, and the first "Task" will be performed.
 [https://github.com/WordPress/wordpress-develop/pull/3170]
 The schema traversal might stop at some nodes, and leave sub-trees
 unvisited without performing the "Task", because the nodes are checked
 against the `type`, and are discarded from further traversal if they do
 not have, or have wrong `type`.


 ==== Solution 2

 It uses the more liberal second object definition, and the second "Task"
 will be performed.
 [https://github.com/WordPress/wordpress-develop/pull/3374]
 The schema will be fully traversed, the "Task" will be performed on all
 nodes, because there are no guards in the "Forest part" nor in the "Tree
 part".





 = Solution 1


 This implementation is very similar to the original one, in which we
 assume that we work on valid schema.

 For this function the schema is valid if:
 - has `type`
 - has correct `type` (if it is `array` then it has `items`, and if it is
 `object` then it has some properties).

 **If the schema is invalid, then**

 ==== 1. Throw notice & discard schema from further traversal

 In the **original implementation** if the schema had no `type`, then the
 schema was discarded from further traversal because of the first line
 `$type = (array) $schema['type'];`, and a run-time notice was thrown: `PHP
 Notice:  Undefined index: type in /home/anna/html/wp60/wp-includes/rest-
 api.php on line 3041`

 In the **proposed implementation** I replaced the built-in notice with the
 `_doing_it_wrong()` notice.

 ==== 2. Discard schema from further traversal

 In the **original implementation** if the schema had no correct `type`,
 then the schema was discarded from further traversal because of these
 guards:

 {{{#!php
 <?php
 if ( in_array( 'object', $type, true ) ) {
     ...
 }
 }}}

 {{{#!php
 <?php
 if ( in_array( 'array', $type, true ) ) {
     ...
 }
 }}}

 This remained as is in the **proposed implementation**.

 ==== 3. Skip performing "Task" on schema

 In the **original implementation** if the schema had no `type ⊃ 'object'`
 then the "Task" was skipped. For example the "Task" was not performed on
 this schema:

 {{{#!php
 <?php
 [
     'properties' => [...]
 ]
 }}}


 The "Task" remained as is in the **proposed implementation**.





 = Solution 2


 While in Solution 1 the schema might have sub-trees unvisited because of a
 missing or wrong `type`, in this solution the traversal is full. The
 "Task" will be performed on all nodes of the schema.

 The test cases have some minor differences, but they seem to be
 progressive rather than regressive.

 **I came up with this solution because I think**

 ==== Validation

 Both solutions are silent about a wrong `type` keyword, but they handle
 this situation differently (discarding vs. keeping nodes). This function
 in general should not be responsible for validation, it is done in another
 function where the schema is validated against the value, and notices will
 be thrown.


 ==== Sanitization

 Looking at the `rest-api.php` code it seems to me that we do not sanitize
 the schema (well, besides this very function where we set the
 `additionalProperties` to `false`), and discarding nodes from performing
 the "Task" on them seems like a (silent to the user) sanitization.

 ==== Completeness

 If we assume that this function actually should have control over some
 aspects of validation or sanitization, then I would expect to control all
 aspects.
 For example schema mistakes like these are also remain silent, and we just
 do not know the reason why some parts of the schema is unvisited:

 {{{#!php
 <?php
 [
     'type'       => 'array',
     'properties' => [...]
 ]
 }}}


 or the reverse:

 {{{#!php
 <?php
 [
     'type'  => 'object',
     'items' => [...]
 ]
 }}}


 After some experiments around building a fully featured validation and/or
 sanitization into this function, I chose the other way: let's expand the
 object definition instead, and there is no reason to think about
 validation nor sanitization anymore in this function.


 But I am not aware of all aspects of these changes, and would like to
 understand if the first solution is desired.

 -----


 If the first solution is the correct one, then it might be worth to
 continue reading about how supporting `anyOf`, `allOf`, `noneOf` changes
 the definition of a valid schema.

 Now having base `type` on `anyOf`, `allOf`, `oneOf` schemas allows not
 having `type` on children. But we need to perform the "Task" on children
 too, so they have to have `type`. And that is when and why we pass the
 `$types` in the "Forest part": the children will inherit the `type` from
 their parent.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56494#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform