Handling ForeignKeyConstraintViolationException

[patrik-owly]

Hello, before I create issue, maybe I'm doing something wrong, so I ask here.

I got 3 entities:

Location:

/**
 * @property		int			$id					{primary}
 * @property		Country		$country			{m:1 Country::$locations}
 * @property		string		$name
 * @property-read	string		$nameWithCountry	{virtual}
 */
class Location extends Entity
{

	protected function getterNameWithCountry(): string
	{
		return $this->name . ', ' . $this->country->name;
	}

}

Country:

/**
 * @property int						$id			{primary}
 * @property string						$name
 * @property OneHasMany|array<Location> $locations	{1:m Location::$country}
 */
class Country extends Entity
{

}

Boat:

 * @property		int								$id						{primary}
 * @property		Location						$location				{m:1 Location, oneSided=true}
 * ...
 */
class Boat extends Entity
{
// rest
}

When I remove Location, which is not related to any Boat, everything is fine.
But when I remove Location, which is related to any Boat, I got following exception:
Nextras\Orm\Exception\NullValueException: Property App\Model\Location\Location::$country is not nullable.

Removing logic is here:

try {
	$this->orm->location->removeAndFlush($location);
	// $this->orm->removeAndFlush($location); <- did not helped
	// $location->country->locations->remove($location); <- did not helped as well
} catch (InvalidStateException | ForeignKeyConstraintViolationException $e) {
	$this->flashMessage(
		'Location can not be removed, because is connected to some boat',
		'danger',
	);
}

Exception is printed while rendering table:

$location->country->name;

But at this moment I would expect Location to be already removed.
I never encountered something similar even if we are using ORM widely now.

Any tips? Thank you very much!

[hrach]

Orm needs to know what should happen if the location is removed and yet it is a location for boat:

• Should it be prevented? To do so:

  /**
   * @property int $id {primary}
   * @property OneHasMany|Boats[] $boats {Boat::$location}
   */
  class Location extends Entity

• Or should be the boat removed too? To do so:

  /**
   * @property int $id {primary}
   * @property OneHasMany|Boats[] $boats {Boat::$location, cascade=[persist, remove]}
   */
  class Location extends Entity

• Or should set the boat->location to null? To do se:

  /**
   * @property int $id {primary}
   * @property OneHasMany|Boats[] $boats {Boat::$location}
   */
  class Location extends Entity
  /**
   * @property int $id {primary}
   * @property Location|null $location {Location::$boats}
   */
  class Boat extends Entity

I understand that oneSided had a reason to avoid potential performance issues when loading all relationships. Yet this should be solved by Nextras Orm 4.0 by introducing https://blog.nextras.org/orm-4-optimized-relationship/

[hrach]

More info for cascade: https://nextras.org/orm/docs/4.0/relationships#toc-cascade

[patrik-owly]

Hello and thank you for your reply, but that's not a problem (or it is?), cause I'm already catching ForeignKeyConstraintViolationException (see removing logic) and it works in other cases too. But the problem here is that exception is poiting to $country is not nullable, which doesn't make sense and exception is thrown when Location has to be already removed.

Cascade is configured right here, I would expect ForeignKeyConstraintViolationException to be thrown.

[hrach]

So If I understand you correctly, you expect that cathing the Nextras\Dbal\Exception\ForeignKeyConstraintViolationException exception will leaves Orm entities in correct state, do you? No, catching the exception doesn't "rollback" the state.

• So either correctly rollback the previous state and refreshAll like here https://nextras.org/orm/docs/4.0/unique-constraint
• or rather be "proactive" and check directly, if the location has any boat.

[patrik-owly]

@hrach hello again, so I tried to rollback changes, first without true in $allowOverwrite parameter, it caused exception:
Nextras\Orm\Exception\InvalidStateException: Cannot refresh modified entity, flush changes first or set $allowOverwrite flag to true.

But then, when I changed the value of the $allowOverwrite to true, same exception occured as before:
Nextras\Orm\Exception\NullValueException: Property App\Model\Location\Location::$country is not nullable.

Here is how I modifed the removing logic:

	public function locationGridActionDelete(Location $location): void
	{
		try {
			$this->orm->location->removeAndFlush($location);
		} catch (InvalidStateException | ForeignKeyConstraintViolationException $e) {
			$this->orm->location->getMapper()->rollback();
			$this->orm->refreshAll(true);
			$this->flashMessage(
				'Lokaci nelze odstranit, jelikož je přiřazena nějaké lodi.',
				'danger',
			);
		}
	}

So to be clear:
I try to remove Location -> exception is catched -> rollback is done -> expecting grid to be rendered in unchanged state -> NullValueException occurs for Location::$country

[hrach]

This could probably mean that the refreshAll/rolllback is not working and there is a bug. Will try to create a reproducer.

[patrik-owly]

If you need some more information or assistance, just let me know. Thank you for now!

[hrach]

Could you please verify that this commit fixes it for you?
365a6e5
You can simply copy-past the only needed row - fix in Identity map - and apply it locally:

$this->repository->attach($entity); // entity can be detached because of delete try

[patrik-owly]

@hrach hello, thank you for your time and I'm happy to say you, that it works 😍 with rollback:

try {
   $this->model->removeAndFlush($location);
} catch (ForeignKeyConstraintViolationException $e) {
   $this->model->location->getMapper()->rollback();
   $this->model->refreshAll(true);
}

Without rollback it still says non-sense message as shown on image above:
Property App\Model\Location\Location::$country is not nullable.

But that's not a show-stopper anymore!

[hrach]

Ok, so the proper solution here is really to check the relationships upfront the entity removal

/**
 * @property int $id {primary}
 * @property OneHasMany|Boats[] $boats {Boat::$location}
 */
class Location extends Entity {}

if ($this->location->boats->countStored() > 0) {
	$this->flashMessage(
		'Lokaci nelze odstranit, jelikož je přiřazena nějaké lodi.',
		'danger',
	);
	return;
}

Why it is recommended? Orm models data as entities and their consistency should be checked upfront. Other storage implementation (like non-relational database/storing simple serialized data) won't cover this data inconsistency. So it should be modeled in Orm.

---

The alternative solution is to use DB checks (foreign key in this case). This needs to employ dbal and its transaction handling and let orm reinit afterwards (and it also requires this bug fixed, as aforementioned):

		try {
			$this->orm->removeAndFlush($user);
		} catch (ForeignKeyConstraintViolationException $e) {
			/** @var DbalMapper $mapper */
			$mapper = $this->orm->userStats->getMapper();
			$mapper->rollback();
			$this->orm->refreshAll();
		}

[patrik-owly]

First solution can't be used, because relation to Location is oneSided from Boat, so I can't call $location->boats directly. Locations can be specified not only for Boats, but for Adventures, Courses, too.

I'm using second solution and thats OK!

Thank you, can be closed now.

[hrach]

The point is not have it onesided. Since Orm 4.0 there should be no significant overhead.