doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.93k stars 2.52k forks source link

oneToMany orphanRemoval with unique - constraint violation #6776

Open akomm opened 7 years ago

akomm commented 7 years ago

I currently do not have time for the test, also because this blocks me: #6738. I try to add one asap.

Issue is the following:

Preconditions:

You do the following in a single transaction/flush:

  1. Remove some entities from the many-side
  2. Create new entities on the many-side

When the column value (unique constraint) on the removed entities (1) equals the column value (unique constraint) on the created entities (2) a constraint violation is raised.

Reason: The created entities (2) are INSERTed before the removed entities (1) are DELETEd.

Found a fixed, similar issue, just ManyToMany: #2310

lcobucci commented 7 years ago

@akomm thanks for reporting the issue, we do need a test though (we're also short on time). Send us something when you're available and we discuss on the PR.

akomm commented 7 years ago

I resolved the #6738 issue but could not make a reproduction "quick" and worked around it removing the unique constraint temporary relying on code as long as the app is not in prod. Want to return to the reproduction test when rest is done, so I can enable the constraint again.

julienb-allopneus commented 6 years ago

Hi, We're facing the same problem but we won't just remove the constraint ;-) Anyone working on this topic?

cc @nio-hevea @Myloth

lcobucci commented 6 years ago

@julienb-allopneus could you please send us a failing test case that reproduces that behaviour? It would help us a lot to identify and fix the issue you're describing.

You can find examples on https://github.com/doctrine/doctrine2/tree/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket

julienb-allopneus commented 6 years ago

Hi @lcobucci , I'll work on this today, and keep you informed as soon as I can.

julienb-allopneus commented 6 years ago

Hi guys @lcobucci & @akomm ,

I have written a test in the following PR https://github.com/doctrine/doctrine2/pull/6838 , please have a look and tell me what's next :-) FYI, I have no experience in doctrine contribution and doctrine internals .

Thanks,

Julien.

cc @nio-hevea @Myloth

julienb-allopneus commented 6 years ago

Hi all,

Any news/advice on the test I wrote? @lcobucci , is it ok for you?

Thanks,

Julien.

julienb-allopneus commented 6 years ago

Hi @lcobucci and @akomm ,

Any news on this issue? I guess that we may remove the "Missing tests" label and requalify the issue.

Thanks,

Julien.

toby-griffiths commented 6 years ago

I'm having this same issue on a OnToOne relationship as well (no matter which the owning side it).

Is this related to this issue, or is there some other documentation someone can point me at?

Ocramius commented 6 years ago

@Toby we'd need a test case to verify that.

On 6 Mar 2018 18:38, "Toby Griffiths" notifications@github.com wrote:

I'm having this same issue on a OnToOne relationship as well (no matter which the owning side it).

Is this related to this issue, or is there some other documentation someone can point me at?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/doctrine/doctrine2/issues/6776#issuecomment-370864160, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakO86kAhc5wRGz95LNkqfQUwTU-acks5tbsl4gaJpZM4P6XxY .

lcobucci commented 6 years ago

@Ocramius @toby-griffiths we do have a test case in #6838 - it needs to be updated, sure

toby-griffiths commented 6 years ago

@Ocramius Should I add my case details here, or raise a new ticket?

toby-griffiths commented 6 years ago

Or Raise a new Test Case, as per @lcobucci 's comment?

toby-griffiths commented 6 years ago

(Thanks for the prompt reply, btw)

toby-griffiths commented 6 years ago

@lcobucci / @Ocramius is it possible to add a test case file to that existing Test Case PR (#6838)? Or should I raise a separate PR to include the OneToOne test case?

Ocramius commented 6 years ago

@toby-griffiths yeah, you'd need to send a PR against that fork

toby-griffiths commented 6 years ago

OK. Will do when I can find a moment.

artem328 commented 6 years ago

Any updates on this issue?

toby-griffiths commented 6 years ago

Please advise if this PR needs modifying (& how) as I was unable to submit the PR to the julienb-allopneus/doctrine2 fork.

elkangaroo commented 5 years ago

Is this still being worked on? I am a bit lost about the status here, but this bug is quiet annoying. Please tell me if there is anything I can do to help.

SenseException commented 5 years ago

Failing tests were added and a bugfix PR is needed to make the tests work.

vincentbab commented 5 years ago

I'm also facing this bug and it's quite annoying. I took a look at doctrine's internals but could not find an easy fix for this bug. Doctrine always execute INSERTs before DELETEs. Any ideas on how to fix this ?

sannek8552 commented 5 years ago

@vincentbab seems like they don't want to fix that. I am olso faced with that issue and trying to find workaround

akomm commented 5 years ago

@sannek8552 can't blame them, they do it in their free time, there is no paid core team supported by some company AFAIK, like in many other OOS projects.

egor-dev commented 5 years ago

I'm also facing with this :(

beberlei commented 4 years ago

This is a long standing bug that should have an even older ticket (I remember it was open during 2.0 beta already). The problem is that we couldn't re-order insert after deletes for some other reason that I forgot at the moment.

kirkmadera commented 3 years ago

I had a similar use case. I think it's better to handle it manually, until Doctrine is able to handle this more gracefully.

This method prevents deletes/inserts when an update will do the job also.

  1. Loop through all existing records, find matches and update them
  2. Remove all unmatched records
  3. Loop through all updates and create records for the ones that were not matched.

Example dealing with currencies:

            foreach ($currency->getExchangeRates() as $exchangeRate) {
                foreach ($data['exchangeRates'] as $exchangeRateData) {
                    if ($exchangeRate->getCurrencyTo()->getCode() == $exchangeRateData['currencyTo']) {
                        $exchangeRate->setRate($exchangeRateData['rate']);
                        continue 2;
                    }
                }

                $currency->removeExchangeRate($exchangeRate);
                $this->entityManager->remove($exchangeRate);
            }

            foreach ($data['exchangeRates'] as $exchangeRateData) {
                foreach ($currency->getExchangeRates() as $exchangeRate) {
                    if ($exchangeRate->getCurrencyTo()->getCode() == $exchangeRateData['currencyTo']) {
                        continue 2;
                    }
                }

                $exchangeRate = new CurrencyExchangeRate();
                $exchangeRate->setCurrencyTo($indexedCurrencies[$exchangeRateData['currencyTo']]);
                $exchangeRate->setRate($exchangeRateData['rate']);
                $currency->addExchangeRate($exchangeRate);
            }
Nek- commented 3 years ago

Since nobody posted it, here you go. This issue is related to https://github.com/doctrine/orm/issues/5109 and you should be aware of the long discussion there.

mpdude commented 1 year ago

It seems that having the DELETEs before the INSERTs would be a fix for this case. But, #10809 adds two examples that show why this cannot easily be done, at least not in general.