Project

General

Profile

bug #6730

Deleting media leads to inconsistent data

Added by Andreas Müller over 1 year ago. Updated about 1 year ago.

Status:
Feedback
Priority:
Highest
Assignee:
Category:
taxeditor
Target version:
Start date:
06/15/2017
Due date:
% Done:

50%

Severity:
critical
Found in Version:

Description

The same problem as described in #6527#note-6 also appears in media bulk editor if you try to delete a media that is used for a protolog (TaxonNameDescription). The check only seems to check for TaxonDescription and SpecimenDescription. But there are multiple places where media can be added and reference. We ALWAYS need a general getReferencingObjects check in case any unexpected reference still exists. This is also important for model changes.

Currently if you delete the image and afterwards you open the name the protolog was attached to you get an ObjectNotFoundException exception which pops up in loop (this should also never happen, for NO exception, please feel free to open a new ticket for this).

login : admin
editor version : 4.8.0.201706150749
server : test.e-taxonomy.eu (edit-test) / rem_conf_am
schema version : 4.7.0.0.201710040000
os : Windows Server 2012 R2 6.3 amd64
java : 1.8.0_121
org.eclipse.swt.SWTException: Failed to execute runnable (eu.etaxonomy.taxeditor.remoting.CdmEagerLoadingException: org.hibernate.ObjectNotFoundException: No row with the given identifier exists: [eu.etaxonomy.cdm.model.media.Media#221])
    at org.eclipse.swt.SWT.error(SWT.java:4533)
    at org.eclipse.swt.SWT.error(SWT.java:4448)
    at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185)
    at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4211)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3827)
    at org.eclipse.jface.window.Window.runEventLoop(Window.java:818)
    at org.eclipse.jface.window.Window.open(Window.java:794)
    at org.eclipse.jface.dialogs.ErrorDialog.open(ErrorDialog.java:346)
    at eu.etaxonomy.taxeditor.model.MessagingUtils$1.run(MessagingUtils.java:279)
    at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
    at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:182)
    at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4211)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3827)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:1121)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1022)
    at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:150)
    at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:693)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
    at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:610)
    at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:148)
    at eu.etaxonomy.taxeditor.Application.start(Application.java:24)
    at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:673)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:610)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1519)
Caused by: eu.etaxonomy.taxeditor.remoting.CdmEagerLoadingException: org.hibernate.ObjectNotFoundException: No row with the given identifier exists: [eu.etaxonomy.cdm.model.media.Media#221]
    at org.hibernate.collection.internal.AbstractPersistentCollection.remoteInitialize(AbstractPersistentCollection.java:1358)
    at org.hibernate.collection.internal.AbstractPersistentCollection.initialize(AbstractPersistentCollection.java:627)
    at org.hibernate.collection.internal.AbstractPersistentCollection.read(AbstractPersistentCollection.java:167)
    at org.hibernate.collection.internal.AbstractPersistentCollection.readSize(AbstractPersistentCollection.java:184)
    at org.hibernate.collection.internal.PersistentList.size(PersistentList.java:114)
    at eu.etaxonomy.taxeditor.ui.section.name.ProtologueElement.setEntity(ProtologueElement.java:77)
    at eu.etaxonomy.taxeditor.ui.section.name.ProtologueElement.setEntity(ProtologueElement.java:1)
    at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionElement.<init>(AbstractEntityCollectionElement.java:106)
    at eu.etaxonomy.taxeditor.ui.section.name.ProtologueElement.<init>(ProtologueElement.java:60)
    at eu.etaxonomy.taxeditor.ui.element.CdmFormFactory.createEntityCollectionElement(CdmFormFactory.java:2702)
    at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.createElementComposite(AbstractEntityCollectionSection.java:263)
    at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.createDynamicContents(AbstractEntityCollectionSection.java:251)
    at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.renderContent(AbstractEntityCollectionSection.java:220)
    at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.internalUpdateSection(AbstractEntityCollectionSection.java:203)
    at eu.etaxonomy.taxeditor.ui.section.AbstractEntityCollectionSection.setEntity(AbstractEntityCollectionSection.java:161)
    at eu.etaxonomy.taxeditor.ui.section.name.ProtologueSection.setTaxonBase(ProtologueSection.java:105)
    at eu.etaxonomy.taxeditor.view.detail.CdmSectionPart.setFormInput(CdmSectionPart.java:91)
    at org.eclipse.ui.forms.ManagedForm.setInput(ManagedForm.java:210)
    at eu.etaxonomy.taxeditor.view.AbstractCdmDataViewer.refresh(AbstractCdmDataViewer.java:123)
    at eu.etaxonomy.taxeditor.view.AbstractCdmDataViewer.setInput(AbstractCdmDataViewer.java:109)
    at eu.etaxonomy.taxeditor.view.detail.DetailsViewer.setInput(DetailsViewer.java:161)
    at eu.etaxonomy.taxeditor.view.detail.DetailsViewPart.showViewer(DetailsViewPart.java:268)
    at eu.etaxonomy.taxeditor.view.detail.DetailsViewPart.selectionChanged_internal(DetailsViewPart.java:136)
    at eu.etaxonomy.taxeditor.view.AbstractCdmEditorViewPart$DelaySelection.run(AbstractCdmEditorViewPart.java:52)
    at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
    at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:182)
    ... 31 more
Caused by: org.hibernate.ObjectNotFoundException: No row with the given identifier exists: [eu.etaxonomy.cdm.model.media.Media#221]
    at org.hibernate.boot.internal.StandardEntityNotFoundDelegate.handleEntityNotFound(StandardEntityNotFoundDelegate.java:28)
    at org.hibernate.event.internal.DefaultLoadEventListener.load(DefaultLoadEventListener.java:227)
    at org.hibernate.event.internal.DefaultLoadEventListener.proxyOrLoad(DefaultLoadEventListener.java:278)
    at org.hibernate.event.internal.DefaultLoadEventListener.doOnLoad(DefaultLoadEventListener.java:121)
    at org.hibernate.event.internal.DefaultLoadEventListener.onLoad(DefaultLoadEventListener.java:89)
    at org.hibernate.internal.SessionImpl.fireLoad(SessionImpl.java:1129)
    at org.hibernate.internal.SessionImpl.internalLoad(SessionImpl.java:1022)
    at org.hibernate.type.EntityType.resolveIdentifier(EntityType.java:632)
    at org.hibernate.type.EntityType.resolve(EntityType.java:424)
    at org.hibernate.type.EntityType.nullSafeGet(EntityType.java:262)
    at org.hibernate.persister.collection.AbstractCollectionPersister.readElement(AbstractCollectionPersister.java:833)
    at org.hibernate.collection.internal.PersistentList.readFrom(PersistentList.java:382)
    at org.hibernate.loader.Loader.readCollectionElement(Loader.java:1353)
    at org.hibernate.loader.Loader.readCollectionElements(Loader.java:892)
    at org.hibernate.loader.Loader.getRowFromResultSet(Loader.java:738)
    at org.hibernate.loader.Loader.processResultSet(Loader.java:972)
    at org.hibernate.loader.Loader.doQuery(Loader.java:930)
    at org.hibernate.loader.Loader.doQueryAndInitializeNonLazyCollections(Loader.java:336)
    at org.hibernate.loader.Loader.doList(Loader.java:2611)
    at org.hibernate.loader.Loader.doList(Loader.java:2594)
    at org.hibernate.loader.Loader.listIgnoreQueryCache(Loader.java:2423)
    at org.hibernate.loader.Loader.list(Loader.java:2418)
    at org.hibernate.loader.hql.QueryLoader.list(QueryLoader.java:501)
    at org.hibernate.hql.internal.ast.QueryTranslatorImpl.list(QueryTranslatorImpl.java:371)
    at org.hibernate.engine.query.spi.HQLQueryPlan.performList(HQLQueryPlan.java:216)
    at org.hibernate.internal.SessionImpl.list(SessionImpl.java:1326)
    at org.hibernate.internal.QueryImpl.list(QueryImpl.java:87)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.bulkLoadLazyCollections(AdvancedBeanInitializer.java:526)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.bulkLoadLazies(AdvancedBeanInitializer.java:435)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNodeNoWildcard(AdvancedBeanInitializer.java:244)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNode(AdvancedBeanInitializer.java:125)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNodeRecursive(AdvancedBeanInitializer.java:105)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeNodeRecursive(AdvancedBeanInitializer.java:107)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initializeAll(AdvancedBeanInitializer.java:85)
    at eu.etaxonomy.cdm.persistence.dao.initializer.AdvancedBeanInitializer.initialize(AdvancedBeanInitializer.java:57)
    at eu.etaxonomy.cdm.persistence.dao.hibernate.common.CdmEntityDaoBase.load(CdmEntityDaoBase.java:665)
    at eu.etaxonomy.cdm.persistence.dao.hibernate.common.CdmGenericDaoImpl.initializeCollection(CdmGenericDaoImpl.java:868)
    at eu.etaxonomy.cdm.api.service.CommonServiceImpl.initializeCollection(CommonServiceImpl.java:351)
    at sun.reflect.GeneratedMethodAccessor2070.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:302)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:190)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:157)
    at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:99)
    at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:281)
    at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:96)
    at 
   ...


Related issues

Related to Edit - bug #6915: Delete does not work in media view for taxon Closed 08/15/2017

Associated revisions

Revision 74eabaa7 (diff)
Added by Katja Luther over 1 year ago

ref #6730: first quick fix, to avoid deleting media referenced somewhere else.

Revision 7620acb4 (diff)
Added by Katja Luther over 1 year ago

ref #6730: fix delete of media

Revision 2b6e8d64 (diff)
Added by Katja Luther over 1 year ago

ref #6730: fix delete of media

Revision f35bd02a (diff)
Added by Patrick Plitzner over 1 year ago

ref #6730 Adapt to cdmlib changes

Revision 3544255d (diff)
Added by Katja Luther about 1 year ago

ref #6730: minor changes in delete handler of bulk editor

Revision 8081ee07 (diff)
Added by Katja Luther about 1 year ago

ref #6730: set result to Abort if media is used elsewhere

Revision 1e29f8c7 (diff)
Added by Katja Luther about 1 year ago

ref #6730: fix delete media if it is used in taxonnamedescription

Revision 602c45a9 (diff)
Added by Katja Luther about 1 year ago

ref #6730:adapt the messages

Revision 64e89f2c (diff)
Added by Katja Luther about 1 year ago

ref #6730: updated objects need to be saved if media is deleted

Revision b8489aa9 (diff)
Added by Katja Luther about 1 year ago

fix #6730: handle description elements of taxa, names and specimen all the same in media delete

Revision d8f50ab8 (diff)
Added by Andreas Müller about 1 year ago

ref #6730 remove some further redundant code in MediaServiceImpl.delete

Revision a509a0ab (diff)
Added by Andreas Müller about 1 year ago

ref #6730 try to remove compilation problem in MediaServiceImpl.delete

Revision 379f6670 (diff)
Added by Andreas Müller about 1 year ago

ref #6730 move updatedObject decleration into loop

Revision 2a44d1ef (diff)
Added by Andreas Müller about 1 year ago

ref #6730 further simplify/unify MediaServiceImpl.delete code

Revision 14e45274 (diff)
Added by Andreas Müller about 1 year ago

ref #6730 try to fix compilation error

Revision 223b2cd6 (diff)
Added by Katja Luther about 1 year ago

approve media delete for multiple referenced media

Revision 36505ad4 (diff)
Added by Katja Luther about 1 year ago

remove possibility of delete media of media specimen

History

#1 Updated by Andreas Müller over 1 year ago

  • Description updated (diff)

#2 Updated by Andreas Müller over 1 year ago

  • Description updated (diff)

#3 Updated by Andreas Müller over 1 year ago

I also do not fully understand why only the 2 explicit usecases "TaxonDescription" and "SpecimenDescription" are handled in the configurator. We either need to have only one general option "if not used elsewhere" or we have to add a 3rd option "if not used at any other place" if we for some reason want to keep these 2 explicit options.

See #6527 for other solutions (general we should try to use the same options throughout the application if somehow possible)

#4 Updated by Katja Luther over 1 year ago

  • Target version changed from Release 4.8 to Release 4.10

#5 Updated by Patrick Plitzner over 1 year ago

  • Related to bug #6915: Delete does not work in media view for taxon added

#6 Updated by Katja Luther over 1 year ago

  • Status changed from New to Resolved

now there are three options for deletion:

  • only remove from gallery
  • delete if not used somewhere else
  • delete from everywhere

and if it is used anywhere else and should only if it not used somewhere else it is only removed from the image gallery.

please review

#7 Updated by Katja Luther over 1 year ago

  • Assignee changed from Katja Luther to Andreas Müller
  • % Done changed from 0 to 80

#8 Updated by Andreas Müller about 1 year ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Andreas Müller to Katja Luther
  • % Done changed from 80 to 40

The original problem (deleting a media from BULK editor which is e.g. referenced by a TaxonNameDescription via protologue) is not yet solved. It is still possible to delete a media that is referenced.
In the configurator only 1 option was shown ("if not used elsewhere"). I did not check this checkbox. The delete was successful anyway and showed the description (recursive) error message.

#9 Updated by Katja Luther about 1 year ago

Andreas Müller wrote:

The original problem (deleting a media from BULK editor which is e.g. referenced by a TaxonNameDescription via protologue) is not yet solved. It is still possible to delete a media that is referenced.
In the configurator only 1 option was shown ("if not used elsewhere"). I did not check this checkbox. The delete was successful anyway and showed the description (recursive) error message.

The message was wrong because the isDeletable method returned ok, now it returns Abort and the message is correct and the line is not deleted.

#10 Updated by Katja Luther about 1 year ago

  • Status changed from Feedback to Resolved

now it should work as aspected.

#11 Updated by Andreas Müller about 1 year ago

  • Assignee changed from Katja Luther to Andreas Müller

#12 Updated by Andreas Müller about 1 year ago

The "Delete" dialog now offers "Delete" or "Überspringen" (sorry don't know what is shown in English version). Why not "Cancel"?

#13 Updated by Andreas Müller about 1 year ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Andreas Müller to Katja Luther

The general issue that inconsistent data is created and media are deleted if they shouldn't is fixed now.

However, the code to delete not only the media but also attached TextData and Descriptions is critical. E.g. when the media is attached to a normal text data which contains text AND media the text data will be deleted together with the text. This is probably not wanted behavior.

the full deletion might be a good idea for image galleries, but not for normal text data. Probably we always need to check this.

Also I do not understand why the delete routine for TaxonDescription is different to the one for SpecimenDescription and NameDescription. Is this only by mistake or is there a reason? If by mistake we should better extract the code into an own method to avoid redundant and error prone code.

Also at the end of the routine only in case of TaxonBase the updatedObject is added to the result. Is this on purpose?

(please pull before changing code as I did some minor cleanup on the code)

#14 Updated by Andreas Müller about 1 year ago

  • % Done changed from 40 to 60

#15 Updated by Katja Luther about 1 year ago

  • Status changed from Feedback to Resolved
  • % Done changed from 60 to 50

#16 Updated by Andreas Müller about 1 year ago

Is this for review for me? All issues fixed?

#17 Updated by Katja Luther about 1 year ago

  • Assignee changed from Katja Luther to Andreas Müller

hopefully yes, I moved the handling of the textdata in a separate method, check for text in textdata and added all updated objects to the result.

#18 Updated by Andreas Müller about 1 year ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Andreas Müller to Katja Luther

I did some further code cleaning (d8f50ab8), please check if you think this is correct.

While doing so I realized that

result.addUpdatedObject(updatedObject);

and all the other "updatedObject" related code is handled after

for (CdmBase ref: references){}

This to me looks strange, as only the final updatedObject will be added. Shouldn't this be done for each updatedObject. Could you check if this needs to be changed?

#19 Updated by Katja Luther about 1 year ago

Andreas Müller wrote:

I did some further code cleaning (d8f50ab8), please check if you think this is correct.

While doing so I realized that

result.addUpdatedObject(updatedObject);

and all the other "updatedObject" related code is handled after

yes you are right, I moved this part of code into the for loop

for (CdmBase ref: references){}

This to me looks strange, as only the final updatedObject will be added. Shouldn't this be done for each updatedObject. Could you check if this needs to be changed?

#20 Updated by Katja Luther about 1 year ago

  • Status changed from Feedback to Resolved
  • Assignee changed from Katja Luther to Andreas Müller

#21 Updated by Andreas Müller about 1 year ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Andreas Müller to Katja Luther

This looks good now.

However, I tried to further simplify/unify the code and to set updatedObject always to null for each loop. See 2 commits. Please review if you think this is correct.

#22 Updated by Andreas Müller about 1 year ago

While doing the refactoring I realized that I don't think we should allow Media being removed from MediaSpecimen. A MediaSpecimen is defined by it's media, therefore removing it should not be allowed. Or have we discussed this differently in the past?

#23 Updated by Katja Luther about 1 year ago

the refactored code looks fine.

you are right a media specimen without media does not make sense. I don't know anymore what we decided in the past. I would add it to the isDeletable and delete method

#24 Updated by Andreas Müller about 1 year ago

By the way there are a couple of other places where Media is used. E.g. DefinedTermBase.getMedia, AmplificationResult.getMedia, Sequence.getContigFile + getPherograms, MediaKey (used whereever keys are usable) and maybe more.

What is the expected behavior (of current code and in general) for these Media. Are they also deleted or do we get a warning that the media can't be deleted? If the later than we definitely should remove MediaSpecimen from the list of deletable media and maybe adapt the configuration text to something like "Delete from taxon and specimen descriptions or if used as protologue for taxon names"

#25 Updated by Andreas Müller about 1 year ago

Sorry if I have even one more issue. Looking at the code I wonder if the service.update(updatedObject) command is really necessary. My understanding of update() is that the object is added(reattached) to the current session. But here, the updated object seems to be always loaded within the current session, so no update (with cascading etc.) is needed. The changes to the updatedObject will always be saved if the session/transaction is committed.

Is there a certain reason why we use update here?

#26 Updated by Katja Luther about 1 year ago

Andreas Müller wrote:

By the way there are a couple of other places where Media is used. E.g. DefinedTermBase.getMedia, AmplificationResult.getMedia, Sequence.getContigFile + getPherograms, MediaKey (used whereever keys are usable) and maybe more.

What is the expected behavior (of current code and in general) for these Media. Are they also deleted or do we get a warning that the media can't be deleted? If the later than we definitely should remove MediaSpecimen from the list of deletable media and maybe adapt the configuration text to something like "Delete from taxon and specimen descriptions or if used as protologue for taxon names"

The code now blocks the deletion (isDeletable returns Abort), may be have to discuss this again.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)