automapper icon indicating copy to clipboard operation
automapper copied to clipboard

[RFC] Treat some `ArrayAccess` classes like `array` or `stdClass`

Open GromNaN opened this issue 1 year ago • 6 comments

This is not a final implementation, just the way to expose the issue.

When querying MongoDB, documents are returned as MongoDB\BSON\Document. This generic class implements ArrayAccess, it can contain any field as an array or stdClass.

In order to support denormalization from MongoDB Documents to class, I need to modify the code of the library to add exception on top of array and stdClass. Instead of being specific to MongoDB classes or wide to all ArrayAccess implementation, a config is necessary to set the list of classes to handle as arrays.

Why would this be interesting when you can convert documents to array? Because with MongoDB\BSON\Document, field values are read only when they are accessed.

  • [ ] Add tests
  • [ ] Add doc

GromNaN avatar May 16 '24 16:05 GromNaN

I like the idea of supporting ArrayAccess.

However, It may be useful to handle all array access classes without having to define all possible classes by doing the following thing :

  • If a target implements \ArrayAccess (is_a) and a source property is not found in it we use the array access methods to write it
  • If a source implements \ArrayAccess (is_a) and a target property is not found in it we try to read from the array access

Then we could do something like :

class Foo
{
    public string $foo = 'foo'; 
    
    public string $bar = 'bar';
}

class Bar extends \ArrayObject
{
    public string $foo;
    
    // ...
}

$automapper->map(new Foo(), Bar::class));

var_dump($bar->foo); // foo
var_dump($bar['bar']); // bar

This will map the foo field and the bar field will be in the array storage

There will be no need to defined a list of class (however we could implement this feature as an opt-in so we don't break existing code that use ArrayAccess)

I believe this should work with Document of MongoDB extension also

joelwurtz avatar May 17 '24 10:05 joelwurtz

That would provide a partial list of supported properties as source or target, which something not supported currently, that raise a lot of questions. Do you have an example of such class that would have properties as class attribute and using ArrayAccess. That seems like a very bad design.

I'm using a list of supported classes because some Model classes could implementation ArrayAccess in order to serve their actual properties.

GromNaN avatar May 17 '24 11:05 GromNaN

Do you have an example of such class that would have properties as class attribute and using ArrayAccess. That seems like a very bad design.

I know that some apis or others lib provide this design in order to support extra attributes / data into an object, as an example symfony provide that for their Event

I'm using a list of supported classes because some Model classes could implementation ArrayAccess in order to serve their actual properties.

We could avoid using those classes by using the ignore property of the MapTo / MapFrom attribute so don't think it's a problem

joelwurtz avatar May 17 '24 12:05 joelwurtz

I'm afraid it won't work. ArrayObject has properties that are not part of the data:

$extractor = new Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor();
$properties = $extractor->getProperties(\ArrayObject::class);
// array:4 [
//   0 => "arrayCopy"
//   1 => "flags"
//   2 => "iterator"
//   3 => "iteratorClass"
// ]

Same for MongoDB\BSON\Document:

$extractor = new Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor();
$properties = $extractor->getProperties(\MongoDB\BSON\Document::class);
// array:4 [
//   0 => "iterator"
// ]

We don't want them to conflict with actual values provided using ArrayAccess. And we never want this properties to be used.

GromNaN avatar May 17 '24 13:05 GromNaN

Given this RFC, which challenges ArrayAccess, it might be better not to rely on it.

GromNaN avatar Jun 04 '24 08:06 GromNaN

Sorry for the late answer, i was struggling on what's best here and i think i have a proper solution.

We still want in the future to allow a user to write something like this :

class MyConfigurationObject implements ArrayAccess
{
    public $foo;

    // ... implementation
}

And the mapper would map foo and extra properties if available.

However, like you said, in some cases we don't want to extract some properties, although there is the possibility to ignore them, it's may be difficult to add when it's an external class.

The overall goal would be to allow to set a strategy for a source and target like this:

  • class strategy : extract properties with property info
  • array strategy : add properties not present in source/target from target/source by using array access
  • dynamic strategy : add properties not present in source/target from target/source by using property access (like class implementing __get / __set or \stdClass)

Strategies are cumulable (except for having array + dynamic), so someone can tell this class should be class + array strategies, or only array strategy, etc ...

We would also provide sane defaults:

  • User class => class strategy
  • User class + ArrayAccess => class + array strategies
  • Internal class + ArrayAccess => array strategy (this should cover your use case since i believe MongoDB classes are internal coming from an extension)
  • ...

I don't want to force you to write all of this, are you ok if we merge this after the next release and rework it to fit our vision ?

joelwurtz avatar Jun 05 '24 09:06 joelwurtz

@GromNaN i have throw a new PR that should allow this.

It does not allow specifying a list of classes, it just relies on a configuration parameter.

However their could be a specific integration for mongodb that automatically set the this configuration by using the event listener system, something like this :

final class MongoDBListener {
    
    public function __invoke(GenerateMapperEvent $event): void
    {
        if ($event->mapperMetadata->sourceReflectionClass?->getName() === MongoDB\BSON\Document::class) {
            $event->allowExtraProperties ??= true;
        }
        
        // handle others classes ...
    }
}

joelwurtz avatar Mar 30 '25 11:03 joelwurtz

I will close this, since the other have been merged, thanks @GromNaN for the PR and the use case, should be available on next version

joelwurtz avatar Apr 03 '25 09:04 joelwurtz