Constructor arguments are not validated when constructing object types from flat types

[dktapps]

When bStrictObjectTypeChecking is false, a property of type object might have its constructor invoked with the value provided by the JSON if it wasn't compatible, which is useful for hydrating stuff like DateTime.

However, the code does not check for the following error conditions:

• Whether the class actually has a constructor (otherwise the argument won't be handled)
• Whether the constructor has >0 accepted arguments (otherwise the argument won't be handled)
• Whether the constructor requires no more than 1 argument (otherwise it will crash)
• Whether the constructor accepts the type of argument given without bailing out
• Whether the class is actually a JsonMapper model with @required properties which won't be set by invoking the constructor (violating expectations of user code)

An example of the last case in particular that bit me on the ass:

ClientData.php:

<?php

declare(strict_types=1);

/**
 * Model class for LoginPacket JSON data for JsonMapper
 */
final class ClientData{
	/**
	 * @var ClientDataPersonaSkinPiece[]
	 * @required
	 */
	public array $PersonaPieces;
}

ClientDataPersonaSkinPiece.php:

<?php

declare(strict_types=1);

/**
 * Model class for LoginPacket JSON data for JsonMapper
 */
final class ClientDataPersonaSkinPiece{
	/** @required */
	public string $PieceId;

	/** @required */
	public string $PieceType;

	/** @required */
	public string $PackId;

	/** @required */
	public bool $IsDefault;

	/** @required */
	public string $ProductId;
}

When used in the following manner:

$mapper = new JsonMapper();
$mapper->bExceptionOnMissingData = true;
$mapper->map(json_decode('{"PersonaPieces":["test"]}'), new ClientData());

results in the following unexpected output:

object(ClientData)#6 (1) {
  ["PersonaPieces"]=>
  array(1) {
    [0]=>
    object(ClientDataPersonaSkinPiece)#9 (0) {
      ["PieceId"]=>
      uninitialized(string)
      ["PieceType"]=>
      uninitialized(string)
      ["PackId"]=>
      uninitialized(string)
      ["IsDefault"]=>
      uninitialized(bool)
      ["ProductId"]=>
      uninitialized(string)
    }
  }
}

[SvenRtbg]

I would argue that this is totally expected. The task of this json mapper is to move data out of a JSON string into objects. It does not have the task to validate if the data in the JSON string is actually suitable to be passed into the objects - especially complain that public properties that have a random @required comment attached are actually populated.

So either you apply a JSON schema validation step before the mapping happens, or you apply a validation of the result, or you create code that is able to work on the reality statement that was made by the JSON string only providing partial data.

Update: I think I'll try to understand the situation a bit more now...

[cweiske]

I concur with @SvenRtbg here.
JsonMapper will not catch all the possible problems. If you need stricter validation of incoming data, use a JSON schema and validate the data against it.

[SvenRtbg]

However... I stepped on some inconsistencies handling null values in various use cases, and I hope to address them in a future PR. First though I want to get the test files organized, then deal with the test description (it lacks describing the use cases).

[dktapps]

Seems I missed off what I expected here - I expected that a JsonMapper_Exception would be thrown, because the string "test" cannot be used to hydrate ClientDataPersonaSkinPiece correctly - it has no constructor to handle the argument - leading to all its fields being unexpectedly uninitialized (as opposed to an error being thrown).

The fields being uninitialized (and no error being generated) leads to unpredictable behaviour since the @required tags of the class are not respected when this kind of invalid data is given.

[SvenRtbg]

This seems to in a way be a superset of the NULL value handling in #233 - an array entry not exactly matching the required target object structure.

[dktapps]

This isn't specific to arrays, but it's particularly difficult to workaround for arrays due to bStrictObjectTypes not being enforced on array elements. (Spoke too soon - looks like #225 was merged since I last looked at my notifications.)

A bunch of extra checks would be needed here:

jsonmapper/src/JsonMapper.php

Line 729 in 2868e36

return $reflectClass->newInstance();

I looked into doing it myself, but I ran into some headaches with validating type compatibility in the constructor inputs. The other stuff (checking presence of constructor, required number of args etc) should be fairly trivial.

[cweiske]

Released with v5.0.0.