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

[WIP] Nullable embedded objects. #1275

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
@lcobucci
Copy link
Member

lcobucci commented Jan 21, 2015

The idea was to have a simple (and clean) way to override the nullable property of embedded objects, so I've
added the nullable attribute on the @Embedded annotation and it have 3 possible values:

  • NULL: The nullable option that was defined on the attributes of the embeddable class won't be overriden;
  • TRUE: All attributes of the embeddable class will be marked as nullable and the embeddable instance only would be created when data is not NULL;
  • FALSE: All attributes of the embeddable class will be marked as non-nullable.

There's a lot of things to be improved (mostly on UnitOfWork), but it's fully working with basic tests as you can see on ValueObjectsTest::testCRUDOfNullableEmbedded() case.

I would appreciate a lot your opinions!

Initial work of nullable embedded objects.
There's a lot of things to be improved (mostly on UnitOfWork), but it's fully working with basic tests.
The idea was to have a simple (and clean) way to override the nullable property of embedded objects, so I've
added the nullable attribute on the Embedded annotation and it have 3 possible values:

- NULL: The nullable option that was defined on the attributes of the embeddable class won't be overriden;
- TRUE: All attributes of the embeddable class will be marked as nullable;
- FALSE: All attributes of the embeddable class will be marked as non-nullable.

The usage is quite simple and can be seen at the ValueObjectsTest::testCRUDOfNullableEmbedded() test case.
@doctrinebot

This comment has been minimized.

Copy link

doctrinebot commented Jan 21, 2015

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3529

We use Jira to track the state of pull requests and the versions they got
included in.

@@ -242,6 +243,48 @@ public function testThrowsExceptionOnInfiniteEmbeddableNesting($embeddableClassN
));
}
public function testCRUDOfNullableEmbedded()

This comment has been minimized.

@Ocramius

Ocramius Jan 24, 2015

Member

@lcobucci consider splitting this test into smaller tests depending on each other via @depends

This comment has been minimized.

@lcobucci

lcobucci Jan 24, 2015

Member

Perfect, I followed the other test case (testCRUD). But using @depends is way better.

This comment has been minimized.

@lcobucci

lcobucci Jan 24, 2015

Member

I'll also change the name of the classes that I've created on that file to match the issue on JIRA (DDC-3529)

* @param array $data
* @param ClassMetadata $class
*/
private function removeNullableEmbeddedReferences(array &$data, ClassMetadata $class)

This comment has been minimized.

@lcobucci

lcobucci Jan 24, 2015

Member

@Ocramius I've tried to keep the things simple here, but if we have a huge amount of data or a large result set I think we might have a little delay to process. I just don't have any idea to improve this =/

This comment has been minimized.

@1ed

1ed Jan 25, 2015

Contributor

What about something like this:

public function createEntity($className, array $data, &$hints = array())
{
    // ...
    //$this->removeNullableEmbeddedReferences($data, $class);

    foreach ($data as $field => $value) {
        if (null === $value && $this->isNullableEmbeddedField($class, $field)) {
            continue;
        }

        if (isset($class->fieldMappings[$field])) {
            $class->reflFields[$field]->setValue($entity, $value);
        }
    }
    // ...
}

private function isNullableEmbeddedField(ClassMetadata $class, $field)
{
    return isset(
        $class->fieldMappings[$field]['declaredField'],
        $class->embeddedClasses[$name = $class->fieldMappings[$field]['declaredField']]
    ) ? true === $class->embeddedClasses[$name]['nullable'] : false;
}

This comment has been minimized.

@lcobucci

lcobucci Jan 25, 2015

Member

Thanks @1ed, its a good alternative. I'll play a lit bit now 😄

This comment has been minimized.

@lcobucci

lcobucci Jan 25, 2015

Member

@Ocramius, @1ed and @beberlei what do you think to extract that logic from UnitOfWork into ClassMetadata? It seems to be proper location, considering that UnitOfWork is already huge.

I thought something like ClassMetadata::updateEntity($entity, array $data) or ClassMetadata::applyData($entity, array $data).

@1ed

This comment has been minimized.

Copy link
Contributor

1ed commented Jan 25, 2015

👍 that would be great if it could fit in 2.5

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Jan 25, 2015

I have implemented @1ed suggestion with that minor refactor.
We should update the interface, but it seems better to me.

@alsar

This comment has been minimized.

Copy link

alsar commented Jan 29, 2015

@lcobucci What is the status of this?
Because it's only one few days left until the feature-freeze for the 2.5 release. It would be great if this could make it into 2.5.

@@ -3310,4 +3316,29 @@ public function getSequencePrefix(AbstractPlatform $platform)
return $sequencePrefix;
}
public function applyData($entity, array $data)

This comment has been minimized.

@Ocramius

Ocramius Jan 29, 2015

Member

This method naming is a bit weird: can you document it?

This comment has been minimized.

@lcobucci

lcobucci Jan 29, 2015

Member

I'll rename it. It is really weird.
What do you think about populateEntity?

public function applyData($entity, array $data)
{
foreach ($data as $field => $value) {
if ($this->shouldUseData($field, $value)) {

This comment has been minimized.

@Ocramius

Ocramius Jan 29, 2015

Member

Is there a way to cache this method call?

This comment has been minimized.

@lcobucci

lcobucci Jan 30, 2015

Member

It can be done, but do you think is needed? I mean, the operations aren't too complex and they're based on attributes that are already caching the information (and only will rely on that attributes when the $value is null)...

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Jan 29, 2015

It is "working" @alsar, but we have to clean some stuff to make it better. I'll send some commits right now.

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 29, 2015

FALSE: All attributes of the embeddable class will be marked as non-nullable.

this case looks weird to me. It changes an optional field to a required one in the embedded object

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Jan 30, 2015

@stof please give us more arguments, let's debate 😄.

In 6340eec I've added a test for that cenario and I believe that in the "country" context is the only way to make the embeddable required, but when dealing with complex value object I agree that it can look weird.

The main question that comes to me is: should we have really complex value objects?

@deeky666

This comment has been minimized.

Copy link
Member

deeky666 commented Jan 31, 2015

I tend to agree with @stof about the FALSE case. Why would one want to make an optional field of the VO required? This really changes the VO semantics and should probably be another VO instead. Would need a good use case IMO otherwise the FALSE option does not make much sense to me.

*/
public function inlineEmbeddable($property, ClassMetadataInfo $embeddable)
public function inlineEmbeddable($property, ClassMetadataInfo $embeddable, $nullable)

This comment has been minimized.

@deeky666

deeky666 Jan 31, 2015

Member

Is that extra parameters really required? Can't you use $this->embeddedClasses[$property]['nullable'] in this method instead?

This comment has been minimized.

@lcobucci

lcobucci Jan 31, 2015

Member

You're right, my mistake.

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Jan 31, 2015

@deeky666 the feature that I need is the TRUE case. As I said before, with simple VO (like the one in ValueObjectsTest) it can make sense but when dealing with complex value objects it is weird.

I can remove the FALSE case so we can merge this feature to doctrine 2.5, and the future will tell us if it was a wise choice.

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Feb 3, 2015

So, do we remove the FALSE case guys?

@alsar

This comment has been minimized.

Copy link

alsar commented Feb 3, 2015

@lcobucci lets remove false for now and get this merged into 2.5.
I can then be added to 2.6 if it will be needed.

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Feb 4, 2015

I've removed the false (by ignoring it), should we raise an error when somene try to use false as value for the nullable option on embedded objects?

@pinkeen

This comment has been minimized.

Copy link

pinkeen commented Jul 21, 2015

I would love this feature!

We have a big project with lots of value object. Right now our code is polluted with null !== SomeVOInstance && !SomeVOInstance->isReallyEmpty() code. It's really easy to forget about the second check and make the code silently introduce a lot of bugs.

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Jul 21, 2015

Guys I think its pointless to discuss this without suggesting a solution to our problem.

When I wrote this PR I thought only about OOP and then @Ocramius said that people could have invalid state by inserting or updating the tables using SQL. I was like: who in f****** hell would be insane enough to insert wrong data into his own database? So I truly understand the frustration, but we can't say that our code is bullet-proof and allow possible issues - or maybe vulnerabilities - be injected into our dependencies.

I think that the only way to have this feature is to create it as an extension. Who "threat mapped objects like stupid data bags that easily go into invalid state" wouldn't use this modified hidrator/metadatafactory and everybody would be happy.

@pinkeen

This comment has been minimized.

Copy link

pinkeen commented Jul 21, 2015

I would be willing to (at least try) implement this feature as an extension. Is there currently an elegant way to inject custom Hydrator/MetadataFactory (as defaults)? Could anybody point me in the right direction?

@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Jul 21, 2015

This is a chicken-egg problem, folks. Not going to happen unless somebody finds a solution to the data consistency problem first.

@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Jul 21, 2015

Also: using the object type is perfectly legit.

@schmittjoh

This comment has been minimized.

Copy link
Member

schmittjoh commented Jul 21, 2015

I think it might be best to use an extra column that has a 1 or 0 to check whether an embedded property should be null or not.

As a side note, you can already do this in user-land to implement this functionality.

@pinkeen

This comment has been minimized.

Copy link

pinkeen commented Jul 21, 2015

@schmittjoh How do I do it transparently in the userland? Could you give me a hint?

@schmittjoh

This comment has been minimized.

Copy link
Member

schmittjoh commented Jul 21, 2015

Something along these lines should work:

class Foo
{
     private $embeddable;
     private $isEmbeddableSet;

     public function getEmbeddable()
     {
         if ( ! $this->isEmbeddableSet) { return null; }

         return $this->embeddable;
     }
}
@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Jul 21, 2015

We actually don't need a modified Hydrator as you can see in the files I've changed.

The MetadaFactory is easily injected on EntityManager configuration as you can see here: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L147

You can also create an extension of the AnnotationDriver and pass it to your configuration too.

@pinkeen

This comment has been minimized.

Copy link

pinkeen commented Jul 21, 2015

@lcobucci thanks, I will look into this.
@schmittjoh thanks, but IMHO this is a non-solution equivalent to the ::isEmpty() method in terms of elegance, readability, maintainability and probability to cause problems.
@Ocramius object may do it for some, but obviously it doesn't work when you need to query VO's fields (my case).

@alsar

This comment has been minimized.

Copy link

alsar commented Jul 22, 2015

Does somebody know how they resolve this issue in Hibernate?

@schmittjoh

This comment has been minimized.

Copy link
Member

schmittjoh commented Jul 22, 2015

@pinkeen, it should be possible to hide the implementation details behind a Doctrine API, so that you do not need to deal with this in user-land, someone would need to create a pull-request though to allow for further discussion

@alsar, last time I checked, it was what I outlined above

@vvh-empora

This comment has been minimized.

Copy link
Contributor

vvh-empora commented Sep 21, 2015

hmmm... very tricky :| is there any known existing extension for this problem?

@fabiocarneiro

This comment has been minimized.

Copy link

fabiocarneiro commented Feb 19, 2016

as the ORM must be able to operate with datasets that are not managed by it.

@Ocramius I don't understand the argument that even with someone manually modifying database the consistency of application should still be valid.

To make an analog, you could in your domain specify that the only allowed values of an object are 1, 2 and 3, and then someone goes to database and update the value to 4. It would still produce invalid state, and could still have catastrophic consequences. That is not related to nulls or multiple columns to represent one value, but to modifying application storage with inconsistent data.

Its also almost the same as saying that if someone delete a mapped column it should still work.

As @lcobucci said, it would be perfect fine to define it nullable in one place and not nullable in other places, and if we eventually follow the idea of having this as a separate table, it would still need to follow the same schema, probably meaning that all columns would be nullable or following the mapping of the embeddable. This basically means that the only way to store embeddables in relational databases is by using columns, not tables.

What I see much more as inconsistency is how it is today, that doctrine builds a object for you even if when you persisted your root object the property was null.

Since the embeddable release I always wanted this feature.

@fabiocarneiro

This comment has been minimized.

Copy link

fabiocarneiro commented Feb 23, 2016

I've been testing this branch and I noticed that it doesn't work with multi-level embeddables. It creates the object no matter if all columns of all nested embeddables are null.

@fprochazka

This comment has been minimized.

Copy link
Contributor

fprochazka commented Feb 23, 2016

What about allowing nullable embedables only for DBs that have constraints?

@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Feb 23, 2016

Nope.
On Feb 23, 2016 13:07, "Filip Procházka" notifications@github.com wrote:

What about allowing nullable embedables only for DBs that have constraints
http://www.postgresql.org/docs/8.1/static/ddl-constraints.html?


Reply to this email directly or view it on GitHub
#1275 (comment).

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Feb 23, 2016

@FabioBatSilva I didn't tested that kind of thing but I don't think would be something difficult to achieve.

I also believe that @Ocramius's statement was good enough to end this (as a standard feature):

That's exactly where I wanted you to go to: this starts to treat the ORM as a gateway to a SQL DBMS used as a NoSQL DBMS :-)
This sort of constraint must be at DB level as well, as the ORM must be able to operate with datasets that are not managed by it.

About @fprochazka's comment:

We cannot expect to push a lot of logic to doctrine's shoulders (it already have too much to carry now). I believe that we need to find a way to solve #4568 and also that we should treat this feature as an extension (if it would be really needed after solving that issue).

@fabiocarneiro

This comment has been minimized.

Copy link

fabiocarneiro commented Feb 23, 2016

@lcobucci I think you are accepting this argument too fast. Allowing mappings to happen doesn't mean that this will make a SQL DBMS to be used as noSQL DBMS. By fixing this problem you don't start storing documents or objects in database. Its still tables and rows.

The thing is that the mapper as we have now, does not support proper mapping of optional objects. The goal of the mapper as I understand is to map any design you can possibly have in your code to relational databases, and having optional (nullable) properties in our objects is far too common to be ignored. Its the same as when we didn't have the embeddables feature and we would choose to design everything with scalar types just to fit the mapper.

Another valid approach, would be allowing the storage of objects that follow the same interface in those embeddable mapped properties (something similar as we have with inheritance mapping for entities) and have either a differentiation column or a class name and then use the null object pattern, but I'd vote for your current approach.

PS: Maybe you mispinged me in the message above?

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Feb 23, 2016

Oops you're correct @fabiocarneiro sorry hehehe.

No, I'm not accepting it too fast (it was a long discussion already 😄).

With #4568 we will be able to use the @AttributeOverrides annotation as @schmittjoh commented on the gist that I created before sending this PR. It may lead to the same problem that @Ocramius mentioned however it will not require any other change on doctrine - and only users that are 100% sure that the database is not handled by other thing rather than doctrine would use it.

@fabiocarneiro

This comment has been minimized.

Copy link

fabiocarneiro commented Feb 24, 2016

@lcobucci as far as I understood, we could use attribute overrides to force all mappings of embeddables to be null, meaning that it could be different configuration in different columns, but that's only 20% of the problem. How does that prevent the hydration of the object that has both columns nullable?

For my value objects, null are inconsistent state, but for the entity that has it as property its not. What I really want is the object not to be created rather than its properties having null.

ORM must be able to operate with datasets that are not managed by it. How exactly by adding this feature we would prevent this statement to be true?

I'd treat this as a BUG. The mapping is valid, you persist the object with a null property and when you retrieve it from the repository, doctrine has created a version that is different from the persisted version. Instead of null in that same property you will get an object in a inconsistent state.

@lcobucci

This comment has been minimized.

Copy link
Member

lcobucci commented Feb 24, 2016

But @fabiocarneiro, that's exactly what's being stated on #4568.
In this PR I've fixed it partially and added the nullable attribute, we need to cover it properly and think about all edge cases and we don't need the nullable modifier because we can achieve it in different ways.

I think we should see with @Ocramius || @guilhermeblanco || @schmittjoh what we could do about that (but on the scope of that issue and not on this PR).

@backbone87

This comment has been minimized.

Copy link

backbone87 commented Apr 16, 2016

There are database features that allow these kind checks in various ways at db level. Just because doctrine can not infer a schema that facilitates such features, doesnt mean it should assume these features are not available. also the mapping to schema conversion is just a secondary feature of doctrine to ease database creation and not to replace each and every (vendor-dependant) db feature.

i would really like this feature to be added.

the eal problem comes when all fields in the embeddable are nullable and the embeddable itself is nullable, because we can not distinquish between a null embeddable and an embeddable containing only null values. a separate boolean-like column is required in this case.

@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Feb 4, 2017

the eal problem comes when all fields in the embeddable are nullable and the embeddable itself is nullable, because we can not distinquish between a null embeddable and an embeddable containing only null values. a separate boolean-like column is required in this case.

That is a sensible solution, although a joined one-to-one is simpler and better for this scenario (and adding a 1-to-1 join is a low cost operation, if indexed correctly)

@Padam87

This comment has been minimized.

Copy link
Contributor

Padam87 commented Mar 23, 2017

Sorry to up this, I read back a lot, but it is possible I have missed something. What about this?:

If all fields are null, check the embeddable configuration. If the configuration specifies that it is nullable, set is as null. If it is not nullable, create the object in every case, even if all fields are null.

$address = new Address(); // all fields nullable, and null
$order->setShippingAddress($address);

$em->persist($order);
$em->flush();
$em->refresh($order);

// null (assuming the configuration specifies nullable=true)
$order->getShippingAddress()

// instanceof Address, empty (assuming the configuration specifies nullable=null)
$order->getShippingAddress()
$order->setShippingAddress(null);

$em->persist($order);
$em->flush(); // exception if nullable=null

About enforcing consistency:
(assuming nullable=true)

If a non null value is received, check the individual field mappings, assert that every required field is set. Otherwise throw an exception.

// not nullable field city is missing
$address = new Address();
$address->setStreet('something');

$order->setShippingAddress($address);

$em->persist($order);
$em->flush(); // exception

I know we can't enforce it on a DB level, but unless you are tampering with the data outside of the ORM, it will be fine. This of course should be mentioned in the docs, but otherwise I don't see it as a problem. There is already a similar issue with single table inheritance...

@enumag

This comment has been minimized.

Copy link
Contributor

enumag commented Dec 13, 2017

@Padam87 pretty much wrote down my own thoughts about this. Can we get a response to his comment? cc @Ocramius @lcobucci

@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Dec 13, 2017

It is a possible implementation, but nullability on all embeddable fields would then be disallowed (need at least one non-null field).

That's a good idea for 3.x, so if anybody wants to help out, start from develop :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment