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

Already on GitHub? Sign in to your account

Illustration of issue noted in DB for GH501 #505

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
4 participants

swquinn commented Feb 19, 2013

Pull request for #501, per @jmikola 's request.

I think that I'm doing this right; unfortunately I hadn't created the gh501 branch previously when working on the test case to illustrate the issue. I made all my changes against my fork's master branch. I'm hoping that creating the branch now is fine. Regardless the only change should be the addition of the GH501Test file. Please let me know if there is anything else I need to do!

Just to re-state what is happening, the test(s) actually pass (IIRC) however in one of the three test cases the data is not persisted as would be expected. This may be "works as designed" and expected behavior, but I was perplexed by it none-the-less.

Thanks again!

@jmikola jmikola commented on the diff Feb 19, 2013

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH501Test.php
+{
+ public function testPersistedAndFlushedEmbeddedDocumentInsideDocumentWithId()
+ {
+ /* PARENT DOCUMENT */
+ $doc = new GH501Document('AlbumDocId');
+ $doc->id = new \MongoId();
+ /* END PARENT DOCUMENT */
+
+ /* EMBEDDED DISCRIMINATED NODE */
+ $node = new GH501Album("TestAlbum", 1);
+ $doc->node = $node;
+ /* END EMBEDDED DISCRIMINATED NODE */
+
+ // Persist and flush the node.
+ $this->dm->persist($node);
+ $this->dm->flush();
@jmikola

jmikola Feb 19, 2013

Owner

There is no need to explicitly persist embedded documents. Additionally, flushing doesn't apply to embedded documents, since it can't exist in the database without the parent.

Logically, persist() tells Doctrine to start managing a new document (mainly so we can track changes). flush() tells Doctrine to immediately write changes for all managed documents back to the database. If you were creating multiple new documents, you'd have to persist() each one but you'd only flush() once.

@swquinn

swquinn Feb 19, 2013

Understood, and I recognize that (the act of persisting an embedded document was due to an error in my code originally which led me to the noted odd behavior). So, if I do persist and embedded document (and subsequently flush it) shouldn't the act of doing a persist()(and possible flush()) on an embedded document be a no-op, since this data should be managed underneath the parent anyway when it is persisted/flushed?

What I'm noticing in the case where I explicitly (albeit, erroneously) call persist() and flush() is that the embedded document is not getting committed to the database only when I also explicitly assign an ID manually. Please let me know if you're not seeing that behavior in the database collection when running the tests.

@jmikola

jmikola Feb 27, 2013

Owner

After I run the tests, I note the following documents present:

> db.GH501Document.find().pretty()
{
    "_id" : ObjectId("512e75cfe84df1de25000001"),
    "hash" : {

    },
    "name" : "AlbumDocId",
    "node" : {
        "type" : "album"
    }
}
{
    "_id" : ObjectId("512e75cfe84df1de25000002"),
    "name" : "AlbumDocNoId",
    "hash" : {

    },
    "node" : {
        "name" : "TestAlbum",
        "value" : NumberLong(1),
        "_doctrine_class_name" : "Doctrine\\ODM\\MongoDB\\Tests\\Functional\\Ticket\\GH501Album",
        "type" : "album"
    }
}
{
    "_id" : ObjectId("512e75cfe84df1de25000003"),
    "name" : "ArticleDoc",
    "hash" : {

    },
    "node" : {
        "name" : "TestArticle",
        "value" : NumberLong(37),
        "_doctrine_class_name" : "Doctrine\\ODM\\MongoDB\\Tests\\Functional\\Ticket\\GH501Article",
        "type" : "article"
    }
}

It definitely looks like mixed document types is kicking, but only when you allow Doctrine to generate the top-level document's ID. I don't have an answer for why it's doing that, though. There could be some incompatibility between discriminator maps being defined on the embedded document classes instead of the parent document's EmbedOne or EmbedMany mapping.

@jmikola jmikola commented on the diff Feb 19, 2013

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH501Test.php
+ }
+}
+
+/** @ODM\Document(db="gh501_test",collection="test") */
+class GH501Document
+{
+ /** @ODM\Id */
+ public $id;
+
+ /** @ODM\String */
+ public $name;
+
+ /** @ODM\Hash */
+ public $hash = array();
+
+ /** @ODM\EmbedOne(name="node") */
@jmikola

jmikola Feb 19, 2013

Owner

Discrimination for embedded documents belongs on the EmbedOne or EmbedMany mapping, not the embedded document class (as described here). You'll want to add the following:

/** @ODM\EmbedOne(
 *    discriminatorField="type",
 *    discriminatorMap={"album"="GH501Album", "article"="GH501Article"}
 *  )
 */
@swquinn

swquinn Feb 19, 2013

I thought I recall reading in that same document that you can omit the discriminating details on an embedded document and Doctrine will automatically add a _doctrine_class_name node, to allow you to persist mixed documents that may not necessarily be part of the discriminator. In the documentation I saw this about mixing document types. That was what I was attempting to illustrate here. I probably should have used two three embedded documents with one not among the discriminated.

@swquinn

swquinn Mar 11, 2013

So I removed the incorrectly placed discriminator map and definition. It sounds like you have one choice or the other: use mixed document types and allow Doctrine to figure it out by the _doctrine_class_name or use an explicitly identified discriminated type map where you know the finite set of documents and their types that will be in it?

@jmikola

jmikola Mar 14, 2013

Owner

I do believe that _doctrine_class_name and discriminator maps are mutually exclusive; however, I'm unsure if that's clearly specified in the documentation.

@jmikola jmikola commented on the diff Feb 19, 2013

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH501Test.php
+ /** @ODM\String */
+ public $name;
+
+ /** @ODM\Hash */
+ public $hash = array();
+
+ /** @ODM\EmbedOne(name="node") */
+ public $node;
+
+ public function __construct($name)
+ {
+ $this->name = $name;
+ }
+}
+
+abstract class GH501BaseNode
@jmikola

jmikola Feb 19, 2013

Owner

Since the $name and $type mappings are duplicated in each inheriting class, you can get away with annotating this as a MappedSuperclass and defining the field mappings here.

@jmikola jmikola and 1 other commented on an outdated diff Feb 19, 2013

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH501Test.php
+
+ public $type;
+
+ public $value;
+
+ public function __construct($name, $value)
+ {
+ $this->name = $name;
+ $this->value = $value;
+ }
+}
+
+/**
+ * @ODM\EmbeddedDocument
+ * @ODM\DiscriminatorField(fieldName="type")
+ * @ODM\DiscriminatorMap({"album"="GH501Album", "article"="GH501Article"})
@jmikola

jmikola Feb 19, 2013

Owner

This is not the place for discriminator mappings (see above comment). If you were to disable the manual persist/flush of the embedded document (which is also out of place but tricks ODM into tracking the document), you'll find that there's an exception attempting to discern the class for the embedded document during hydration (before the find() query returns to the test case). That is because these mappings aren't read at all for embedded documents.

@swquinn

swquinn Feb 19, 2013

So how does this behave with respect to the mixing of document types? Do mixed document types work because they get the special _doctrine_class_name in the document structure and Doctrine can key off of that for Hydration?

@jmikola jmikola commented on the diff Feb 19, 2013

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH501Test.php
+ * @ODM\DiscriminatorMap({"album"="GH501Album", "article"="GH501Article"})
+ */
+class GH501Album extends GH501BaseNode
+{
+ /** @ODM\String */
+ public $name;
+
+ /** @ODM\Int */
+ public $value;
+
+ public $type = 'album';
+
+ public function __construct($name, $value)
+ {
+ parent::__construct($name, $value);
+ $this->type = 'album';
@jmikola

jmikola Feb 19, 2013

Owner

This should be superfluous since you're defining a default on the property itself.

@jmikola jmikola commented on an outdated diff Feb 27, 2013

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH501Test.php
+ /* END EMBEDDED DISCRIMINATED NODE */
+
+ // Now, persist and flush the object you would expect to be operating on.
+ $this->dm->persist($doc);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $doc = $this->dm->getRepository(__NAMESPACE__ . '\GH501Document')->findOneByName('ArticleDoc');
+ $this->assertEquals('ArticleDoc', $doc->name);
+ $this->assertEquals('TestArticle', $doc->node->name);
+ $this->assertEquals('article', $doc->node->type);
+ $this->assertEquals(37, $doc->node->value);
+ }
+}
+
+/** @ODM\Document(db="gh501_test",collection="test") */
@jmikola

jmikola Feb 27, 2013

Owner

I noticed that if tests complete successfully on the first run, they'll fail on the second, due to old data hanging around in this database. It'd be preferable to replace this with simply @ODM\Document, so the main test DB gets used and cleaned up by the base test.

swquinn added some commits Mar 11, 2013

@swquinn swquinn Use default DB and collection for document
Modified GH501's document definition so that the DB and collection used are the default in the test. As per jmikola's suggestion.
9152dba
@swquinn swquinn Remove explicit discriminator mappings
Remove explicit discriminator mappings on the embedded document's class definitions, per jmikola's indication that this is not the proper place for them.
220bc92
Owner

jwage commented Mar 28, 2015

Can we update the test so that it checks for the data that is incorrect and fails?

Member

malarzm commented Jul 30, 2016

Closing since no update was provided for over a year

@malarzm malarzm closed this Jul 30, 2016

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