Skip to content
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

[MVC] ModelRelationField should prevent deletion of referenced item #1897

Closed
fraenki opened this issue Oct 26, 2017 · 5 comments
Assignees
Labels
Projects
Milestone

Comments

@fraenki
Copy link
Member

@fraenki fraenki commented Oct 26, 2017

The ModelRelationField type is a great way to add a reference to another object. I really like it and use it a lot for my plugins.

Unfortunately, it is currently possible to delete the referenced object without any notice or warning. This will leave the configuration in an invalid state, the validation fails from now on and - most importantly - model migrations during an upgrade will fail:

Oct 26 23:11:31 opnsense config[56018]: [OPNsense\HAProxy\HAProxy:actions.action.0981fe29-5ca1-43de-b356-d351c13d8f67.use_backend] Related backend item not found
Oct 26 23:11:31 opnsense config[56018]: Model OPNsense\HAProxy\HAProxy can't be saved, skip ( Phalcon\Validation\Exception: [OPNsense\HAProxy\HAProxy:actions.action.0981fe29-5ca1-43de-b356-d351c13d8f67.use_backend] Related backend item not found  in /usr/local/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php:509 Stack trace: #0 /usr/local/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php(613): OPNsense\Base\BaseModel->serializeToConfig() #1 /usr/local/opnsense/mvc/script/run_migrations.php(56): OPNsense\Base\BaseModel->runMigrations() #2 {main} )

I suggest to disallow the deletion of an object if it's still referenced by a ModelRelationField type.
Would this be possible?

@AdSchellevis

This comment has been minimized.

Copy link
Member

@AdSchellevis AdSchellevis commented Oct 27, 2017

It's a bit tricky to validate, the related field doesn't really know it's related. We should be able to add some glue to validate this state, but I don't think we should change the default behaviour.

@AdSchellevis AdSchellevis self-assigned this Oct 27, 2017
@mimugmail

This comment has been minimized.

Copy link
Member

@mimugmail mimugmail commented Oct 27, 2017

Please CC me when there is a change, I also use it heavily in Quagga and it's quite sensible

@AdSchellevis

This comment has been minimized.

Copy link
Member

@AdSchellevis AdSchellevis commented Nov 8, 2019

Ok, we can solve this in our standard controller, but let's keep it "opt-in" to start with.
The idea is actually quite simple, if the (exact)uuid is found in an xml tag (content), which parent has a uuid attribute itself and a version attribute is found further up in the tree. We can safely assume this item has been referenced.

Add the following in your controller to use:

protected static $internalModelUseSafeDelete = true;

Which will then produce errors like this:

image

For reference I'll leave the path in the exception, so it should be easy to validate.

@AdSchellevis

This comment has been minimized.

Copy link
Member

@AdSchellevis AdSchellevis commented Nov 8, 2019

@mimugmail you wanted to stay posted ^^

@mimugmail

This comment has been minimized.

Copy link
Member

@mimugmail mimugmail commented Nov 8, 2019

Thx, I will test it next week. Have a nice weekend

@fichtner fichtner added this to To Do in 20.1 via automation Nov 8, 2019
@fichtner fichtner moved this from To Do to Done in 20.1 Nov 8, 2019
@fichtner fichtner added this to the 20.1 milestone Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
20.1
  
Done
4 participants
You can’t perform that action at this time.