Mongodb-odm: Partial discriminator-maps not working anymore since 1.2.7

[rieschl]

BC Break Report

| Q | A
|------------ | ------
| BC Break | yes
| Version | 1.2.7

Summary

The changes calculating the discriminator mapping introduced a BC break in our mappings, because the discriminator map in the mapped-superclass only contained one of the documents. That may be "smelly", but it worked before.

Previous behavior

Having a partial discriminator map did automatically add the missing discriminator-mappings using the FQCN as discriminator-mapping value.

Current behavior

The documents not in the discriminator-map can be saved but not fetched anymore.
If I add the other documents or remove the discriminator-map completely it works again.

How to reproduce

Current mapping

Mapped superclass

<doctrine-mongo-mapping
        xmlns="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping"
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
        http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd"
>
    <mapped-superclass
            name="Eventjet\Ticket\AbstractTicket"
            collection="tickets"
            inheritance-type="SINGLE_COLLECTION"
    >
        <field name="id" id="true" type="uuid" strategy="NONE"/>
        <field name="number" fieldName="number" type="string"/>
        <field name="scan_code" fieldName="scanCode" type="string"/>
        <field name="revoked" type="boolean"/>
        <reference-one
                field="ticket_type"
                fieldName="ticketType"
                target-document="Eventjet\Ticket\TicketType\AbstractTicketType"
        />
        <discriminator-field name="type"/>
        <discriminator-map>
            <discriminator-mapping value="standing_room" class="Eventjet\Ticket\StandingRoomTicket"/>
        </discriminator-map>
        <indexes>
            <index name="tickets_revoked_seats">
                <key name="seat_id"/>
                <key name="revoked"/>
            </index>
        </indexes>
    </mapped-superclass>
</doctrine-mongo-mapping>

document mapping contained in discriminator-map:

<doctrine-mongo-mapping xmlns="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping"
                        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                        xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
                        http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
    <document name="Eventjet\Ticket\StandingRoomTicket">
        <field name="event" type="event"/>
    </document>
</doctrine-mongo-mapping>

one of the document mappings not contained in discriminator-map:

<doctrine-mongo-mapping xmlns="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping"
                        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                        xsi:schemaLocation="http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping
                        http://doctrine-project.org/schemas/odm/doctrine-mongo-mapping.xsd">
    <document name="Eventjet\Ticket\Seatmap\SeatTicket">
        <field name="seatmap" type="string"/>
        <field name="seat_id" fieldName="seatId" type="string"/>
    </document>
</doctrine-mongo-mapping>

Consider the following test cases:

    public function testStandingRoomTicketEventIsSaved(): void
    {
        $ticketType = $this->createStandingRoomTicketType();
        $event = $ticketType->getEvent();
        $this->eventRepository->save($event);
        $this->ticketTypeRepository->save($ticketType);
        $ticket = new StandingRoomTicket($ticketType, $event);

        $ticket = $this->saveAndReloadStandingRoomTicket($ticket);

        $this->assertSame($event->getId(), $ticket->getEvent()->getId());
    }

    public function testPersistSeatTickets()
    {
        $seat = ObjectFactory::concreteSeat();
        $seatmap = $seat->getSeatmap();
        $this->getContainer()->get(ConcreteSeatmapRepositoryInterface::class)->save($seatmap);
        $ticketType = ObjectFactory::seatmapCategoryTicketType($seatmap);
        $this->ticketTypeRepository->save($ticketType);
        $ticket = ObjectFactory::seatTicket($ticketType, $seat);

        $loaded = $this->saveAndReloadSeatTicket($ticket);

        $this->assertSame($ticket->getSeat()->getId(), $loaded->getSeat()->getId());
        $this->assertSame($ticket->getTicketType()->getId(), $loaded->getTicketType()->getId());
    }

The first one still workes with 1.2.7, the other doesn't. Adding the discriminator-mapping of the document or removing the whole discriminator-map in the mapped-superclass makes the tests pass again.

[alcaeus]

Yeah, this looks like it would be my fault in #1962. I have to see what I can do here - looks like this is one of those situations where we can't fix one use-case without breaking another.

For the record even though I've already mentioned it in Slack, partial discriminators will be deprecated in the upcoming 1.3 release and dropped entirely in 2.0, so you'll have to update the logic soon.

I'll see if I can fix the issue without breaking more things 😑

[ostrolucky]

Both could be fixed by saving all parent discriminators in metadata, then using $nin specifying all parent discriminators (or discriminators of other classes on same level), instead of current $in with discriminators of current class. This is more complicated approach though and not sure if worth it when use case in this issue is going to be removed in 2.x. Personally I would just close this issue as it's relying on undefined behaviour and reverting #1962 would cause another regression between 1.2.7-1.2.8

[alcaeus]

reverting #1962 would cause another regression between 1.2.7 and 1.2.8

Since it never worked before 1.2.7, I'm willing to take that risk.

Personally I would just close this issue as it's relying on undefined behaviour

Relying on "undefined" behaviour is what happens after a software gets older. Remember, the first beta versions of 1.x are almost 9 years old - people come to rely on certain things, especially when you tag a final release. Whether it's undocumented or not, I don't like breaking people's workflows, no matter how ridiculous it seems. Also keep in mind that ORM exhibits the same behaviour here, so this is not just "relying on undefined behaviour", you could almost assume a pattern here.

Fact is, this regression causes a BC break while updating. This is made worse by the fact that we're only deprecating partial discriminator maps in 1.3 for removal in 2.0, so I can hardly go out and break this now.

I'm making another attempt at fixing this, I'll see if diffing discriminator values from the map produces a fix for the issue that doesn't break valid use cases. If not, this PR is getting reverted today, with 1.2.9 fixing the regression introduced by this patch.

[alcaeus]

We've reverted #1962 in #1978 for the 1.x branch. The fix will be added only to 2.x to avoid the regression with partial discriminator maps.