Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dktapps opened this issue Feb 20, 2024 · 6 comments

Comments

@dktapps
Copy link
Contributor

dktapps commented Feb 20, 2024

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
Copy link
Contributor

SvenRtbg commented Feb 20, 2024

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
Copy link
Owner

cweiske commented Feb 27, 2024

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.

@cweiske cweiske closed this as completed Feb 27, 2024
@SvenRtbg
Copy link
Contributor

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
Copy link
Contributor Author

dktapps commented May 15, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

dktapps commented May 22, 2024

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:

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 added a commit that referenced this issue Jul 20, 2024
BC break!

Passing simple types to the class constructor may lead
to a bunch of unexpected behavior, which can result in crashes
or violate expectations - see the list in the README.

By enabling strict object type checks automatically,
JsonMapper behaves more calculatable.

This is a backwards compatibility break.
The old behavior can be restored by setting the
$bStrictObjectTypeChecking option to `false`.

Resolves: #226
Resolves: #238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants